Make querying which operating system we run a script on more robust

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: armenzg, Assigned: s244sing, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Hello!
Thanks for looking into helping the Mozilla project. Your help is very much appreciated and essential to the sustainability of our support to the Mozilla mission. [1]

In order to contribute with this issue you should follow the "setup" instructions in here:
https://wiki.mozilla.org/Auto-tools/Projects/Mozharness#Setup

= The issue =
In mozharness/base/script.py we have defined ScriptMixin.
In bug 1066700 we added the ability to check if a script is being run on a Mac machine.
We determined that for a function to be robust we might need to check various values (os.name, sys.platform, platform.system()) [2]
In this bug all we want to make is_darwin() more robust and add a similar function for Linux.

From https://bugzilla.mozilla.org/show_bug.cgi?id=1066700#c16
 <jlund> armenzg: based off http://stackoverflow.com/questions/4553129/when-to-use-os-name-sys-platform-or-platform-system I can see why BaseScript.is_windows() uses multiple conditions.
 <jlund> armenzg: seems like the best pattern is to use a combination of platform.system() (which is runtime and probably best for CI) and fallsback on os.name (build configure time)

= Get help =
If you need help or guidance feel free to write a comment in this bug.
You can also chat with us by visiting our IRC channel https://chat.mibbit.com/?url=irc%3A%2F%2Firc.mozilla.org%2F%23ateam

To know more about mozharness read:
https://wiki.mozilla.org/Auto-tools/Projects/Mozharness

To know more about IRC you can watch:
http://codefirefox.com/video/irc

[1] https://www.mozilla.org/en-US/about/manifesto/
[2] http://hg.mozilla.org/build/mozharness/file/default/mozharness/base/script.py#l122

Comment 1

4 years ago
Hi Armen,

I'd like to take this one up. I'll comment here if I have any questions.
(Assignee)

Comment 2

4 years ago
Created attachment 8527461 [details] [diff] [review]
[v1]: Making _is_darwin() more robust and adding _is_linux()

Just picking up on another bug while I wait..

I tested these calls individually in a python interpreter on my linux box and a my macintosh machine. They seem to be fine.

Only concern I have is that os.name.startswith() on both mac and linux returns "posix", do we need to have some additional checks here (maybe adding two conditions, instead of one)?

PS: I noticed that initially the darwin call was named as is_darwin() instead of _is_darwin() [notice the starting underscore], was that a typo considering how _is_windows() was named?
Attachment #8527461 - Flags: review?(armenzg)
Attachment #8527461 - Flags: feedback?(armenzg)
(Reporter)

Comment 3

4 years ago
(In reply to Anurag from comment #1)
> Hi Armen,
> 
> I'd like to take this one up. I'll comment here if I have any questions.

Hello Anurag, welcome to Mozilla!
We hope you get to make an impact and learn lots more about programming.

Unfortunately it seems that Simarpreet already has some working code.

Would you mind having a look in here to find other bugs that interest you?
https://wiki.mozilla.org/Auto-tools/Projects/Mozharness#Good_next_bugs
https://wiki.mozilla.org/ReleaseEngineering/Contribute#First_Projects

I've also picked one for you that I think is not too difficult:
https://bugzilla.mozilla.org/show_bug.cgi?id=965027

Also let me know what interests you besides automation in case you want to try/learn other stuff.
(Reporter)

Updated

4 years ago
Assignee: nobody → s244sing
(Reporter)

Comment 4

4 years ago
Comment on attachment 8527461 [details] [diff] [review]
[v1]: Making _is_darwin() more robust and adding _is_linux()

Review of attachment 8527461 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, it is a typo. Could you also fix it and see if we call it anywhere else in mozharness?

Don't worry about the posix check. You can just remove the check for it.

Once you get back with me with those changes I should be able to land it for you.

Would this be your first code making it into the Mozilla repositories?

::: mozharness/base/script.py
@@ +128,5 @@
>          if os.name == 'nt':
>              return True
> +    def _is_darwin(self):
> +        system = platform.system()
> +        if system in ("Darwin"):

Can you please remove the "system = " line and put it inside of the condition?

Since we don't use the system variable beyond the first conditional check.

@@ +131,5 @@
> +        system = platform.system()
> +        if system in ("Darwin"):
> +            return True
> +        if os.name.startswith("posix"):
> +            return True

Pls remove.

@@ +137,5 @@
> +            return True
> +
> +    def _is_linux(self):
> +        system = platform.system()
> +        if system in ("Linux"):

Same as above. Only one use of system.

@@ +138,5 @@
> +
> +    def _is_linux(self):
> +        system = platform.system()
> +        if system in ("Linux"):
> +            return True

Pls remove.

@@ +142,5 @@
> +            return True
> +        if os.name.startswith("posix"):
> +            return True
> +        if sys.platform.startswith("linux"):
> +            return True

Nit: can you please add an empty line in between functions?
Attachment #8527461 - Flags: review?(armenzg)
Attachment #8527461 - Flags: review+
Attachment #8527461 - Flags: feedback?(armenzg)
Attachment #8527461 - Flags: feedback+
(Assignee)

Comment 5

4 years ago
Created attachment 8528477 [details] [diff] [review]
[v2]: Fixing some issues based on the last review.

Fixing some issues that were raised in the last review.

For now I've removed both of the posix checks from the _is_darwin() and the _is_linux() calls.

Also did a hg grep "_is_darwin()" to find all existing uses of the call. Found a place where it was getting used and fixed the typo in there.
Attachment #8528477 - Flags: review?(armenzg)
Attachment #8528477 - Flags: feedback?(armenzg)
(Reporter)

Comment 6

4 years ago
Comment on attachment 8528477 [details] [diff] [review]
[v2]: Fixing some issues based on the last review.

Review of attachment 8528477 [details] [diff] [review]:
-----------------------------------------------------------------

Yay! We got a winner! :)
I will land it tomorrow.

FYI, you can set only one of the two flags.
Review is when you're happy with the code and believe that is ready to land.
Feedback is when you're not quire ready with the code but you want at least someone to check if you're going in the right direction.
Attachment #8528477 - Flags: review?(armenzg)
Attachment #8528477 - Flags: review+
Attachment #8528477 - Flags: feedback?(armenzg)
(Reporter)

Comment 7

4 years ago
I will NI myself so I will land this tomorrow.
Flags: needinfo?(armenzg)
(Reporter)

Comment 9

4 years ago
Thanks Simarpreet for your help!
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(armenzg)
Resolution: --- → FIXED
(Assignee)

Comment 10

4 years ago
(In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from comment #9)
> Thanks Simarpreet for your help!

Sweet. Thanks for you help as well to get this worked out. Glad to help.
You need to log in before you can comment on or make changes to this bug.