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)

defect
Not set
normal

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
Bug 1209701 - Don't require platforms as input to |./mach try| if set in the environment. r=jgraham
Attachment #8667498 - Flags: review?(james)
Assignee: nobody → cmanchester
Attachment #8667498 - Flags: review?(james)
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.
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.
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 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+
https://hg.mozilla.org/mozilla-central/rev/a50cbda36a49
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: