Closed
Bug 1209701
Opened 9 years ago
Closed 9 years ago
Can't specify platforms in |./mach try| with an environment variable anymore
Categories
(Testing :: General, defect)
Testing
General
Tracking
(firefox44 fixed)
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: chmanchester, Assigned: chmanchester)
References
Details
Attachments
(1 file)
$ echo $AUTOTRY_PLATFORM_HINT linux $ ./mach try testing/mochitest/ --no-push mach try is under development, please file bugs blocking 1149670. Platforms must be specified as an argument to autotry
Assignee | ||
Comment 1•9 years ago
|
||
Bug 1209701 - Don't require platforms as input to |./mach try| if set in the environment. r=jgraham
Attachment #8667498 -
Flags: review?(james)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → cmanchester
Updated•9 years ago
|
Attachment #8667498 -
Flags: review?(james)
Comment 2•9 years ago
|
||
Comment on attachment 8667498 [details] MozReview Request: Bug 1209701 - Don't require platforms as input to |./mach try| if set in the environment. r=jgraham https://reviewboard.mozilla.org/r/20769/#review18613 ::: testing/mach_commands.py:468 (Diff revision 1) > - print("Platforms must be specified as an argument to autotry") > + kwargs["platforms"] = [os.environ['AUTOTRY_PLATFORM_HINT']] Won't this raise KeyError if the environment variable isn't set? That seems like suboptimal error handling. ::: testing/tools/autotry/autotry.py:25 (Diff revision 1) > help='Platforms to run (required if not found in the environment).') If this is supported the environment variable should be documented. So, I'm pretty strongly of the opinion that this feature is more trouble than it's worth. I think that environment variables in general are best avoided because they act like global variables and have the same kinds of problems (e.g. making it hard to debug other people's problems, making commands do different things depending on hidden state rather than just according to their arguments). I also think it's weird that only platforms has an environment variable and not anything else, and that we should try to solve the same problems in a more transparent way. But if this is important to your workflow I'm not going to block it landing.
Assignee | ||
Comment 3•9 years ago
|
||
https://reviewboard.mozilla.org/r/20769/#review18613 > Won't this raise KeyError if the environment variable isn't set? That seems like suboptimal error handling. The argument parser treats "-p" as required if the environment variable isn't present. If I had noticed the removal of this in bug 1193215 I would have raised an issue blocking the review (also suggesting that if we were to remove it, the help string would need to be updated). I don't see why this should be treated any differently from any other regression. This isn't a killer feature or anything, but I think the feature has value because it allows a user to push to try without anything that resembles try syntax. I really don't see how it's any trouble at all beyond the commit to fix it, which has already been written and tested.
Comment 4•9 years ago
|
||
https://reviewboard.mozilla.org/r/20769/#review18613 Well as I said I think the trouble is that environment variables in general are an anti-pattern that should be avoided where possible, and that this specific use of them sits badly with the rest of the API for this utility. The argument that you don't need "anything that resembles try syntax" seems misleading since you still need it, it's just that you are hiding part of it in a shell global. If you would also be happy with a fix that simply removed the erroenous help text I could certainly prepare that. On the other hand, like I said I'm not going to block you from landing this if you really consider it important.
Comment 5•9 years ago
|
||
Comment on attachment 8667498 [details] MozReview Request: Bug 1209701 - Don't require platforms as input to |./mach try| if set in the environment. r=jgraham https://reviewboard.mozilla.org/r/20769/#review18735
Attachment #8667498 -
Flags: review+
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a50cbda36a49
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•