Closed Bug 1404480 Opened 7 years ago Closed 7 years ago

mozinstall on mac is failing if the volume contains a space

Categories

(Testing :: Mozbase, defect)

Product:
Testing
Component:
Mozbase
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

Details

Attachments

(1 file)

Bug 1404480 - Manage hdiutil output when the volume name contains a space
59 bytes, text/x-review-board-request
whimboo
: review+
Details 
05:34:57    ERROR -      for appFile in os.listdir(appDir):
05:34:57    ERROR -  mozinstall.mozinstall.InstallError: Failed to install "/Users/cltbld/tasks/task_1505306020/installer.dmg ([Errno 2] No such file or directory: '/Volumes/Firefox')"

Working on bug 1378834, I noticed that the build on mac is failing if the patch contains a space.

As we are using popen, I am proposing to use that to extract the information instead of doing it in python.
Blocks: 1378834
Assignee: nobody → sledru
Actually, working on the output of hdiutil is too risky. I think we should instead block the usage of a space in MOZ_APP_DISPLAYNAME (we don't have any currently)
Summary: mozinstall on mac is failing if the volume contains a space → Fails the build if MOZ_APP_DISPLAYNAME contains a space
Oh, right...seems that the space is only breaking mac then...
Flags: needinfo?(sledru)
Attachment #8913846 - Flags: review?(mh+mozilla)
(In reply to Sylvestre Ledru [:sylvestre] from comment #4)
> Actually, working on the output of hdiutil is too risky. I think we should
> instead block the usage of a space in MOZ_APP_DISPLAYNAME (we don't have any
> currently)

If my understanding is correct and MOZ_APP_DISPLAYNAME is used both for the name that is displayed in the macOS menu bar and for the file name for the .app bundle, we absolutely have to support spaces if we want to switch to a name with spaces such as "Firefox Nightly".
Sylvestre, would you mind to also file a new bug to get this fixed in mozinstall? Thanks.
Flags: needinfo?(sledru)
Yeah, this was the initial goal of this bug.
I will have a look. I think I know how to fix it.
Flags: needinfo?(sledru)
Summary: Fails the build if MOZ_APP_DISPLAYNAME contains a space → mozinstall on mac is failing if the volume contains a space
Component: Build Config → Mozbase
Product: Core → Testing
Sylvestre, would that patch also fix bug 969334?
Flags: needinfo?(sledru)
I am just updating the parsing. It is better than doesn't address the issues in the bug you are mentioning. It requires more Mac skills.
Flags: needinfo?(sledru)
Blocks: 1404796
Attachment #8913846 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8913846 [details]
Bug 1404480 - Manage hdiutil output when the volume name contains a space

https://reviewboard.mozilla.org/r/185236/#review190658

I'm not familiar with mozinstall and have to defer to someone else for the review here.

::: testing/mozbase/mozinstall/mozinstall/mozinstall.py:283
(Diff revision 6)
>  
>      """
>      try:
> -        proc = subprocess.Popen('hdiutil attach -nobrowse -noautoopen "%s"' % src,
> +        # According to the Apple doc, the hdiutil output is stable and is based on tab
> +        # Therefor, $3 should give us the mounted path
> +        proc = subprocess.Popen('hdiutil attach -nobrowse -noautoopen "%s"|grep /Volumes/|awk \'BEGIN{FS="\t"} {print $3}\'' % src, # noqa

This line is over 99 characters long and fails on try.
Attachment #8913846 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8913846 [details]
Bug 1404480 - Manage hdiutil output when the volume name contains a space

https://reviewboard.mozilla.org/r/185236/#review190658

> This line is over 99 characters long and fails on try.

This is why I added the noqa at the end of the line ;)
Comment on attachment 8913846 [details]
Bug 1404480 - Manage hdiutil output when the volume name contains a space

https://reviewboard.mozilla.org/r/185236/#review190658

You asked me for review and I will do so once I have everything else reviewed which I was asked first, and I had time to check how hdiutils works. I cannot promise it for today. Is that so urgent that you flip between reviewers that often through a single day?

> This is why I added the noqa at the end of the line ;)

And which is not working as Stephen mentioned.
The try was done before I added the noqa. I updated it after seeing the try results
Comment on attachment 8913846 [details]
Bug 1404480 - Manage hdiutil output when the volume name contains a space

https://reviewboard.mozilla.org/r/185236/#review190658

> And which is not working as Stephen mentioned.

This looks fine to me, and it makes that code way more stable. But please reformat those lines as:

        proc = subprocess.Popen('hdiutil attach -nobrowse -noautoopen "%s"'
                                '|grep /Volumes/'
                                '|awk \'BEGIN{FS="\t"} {print $3}\'' % src,

That way we don't need the `# noqa` to stop the linter from checking this line.
Comment on attachment 8913846 [details]
Bug 1404480 - Manage hdiutil output when the volume name contains a space

https://reviewboard.mozilla.org/r/185236/#review191844

r=me with comments addressed.

::: testing/mozbase/mozinstall/mozinstall/mozinstall.py:281
(Diff revision 6)
>      src -- DMG image which has to be extracted
>      dest -- the path to extract to
>  
>      """
>      try:
> -        proc = subprocess.Popen('hdiutil attach -nobrowse -noautoopen "%s"' % src,
> +        # According to the Apple doc, the hdiutil output is stable and is based on tab

Maybe your sentence was cut off here? So it's based on what tab related thing? I assume tab separators?
Attachment #8913846 - Flags: review?(hskupin) → review+
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e07d7bb8d948
Manage hdiutil output when the volume name contains a space r=whimboo
https://hg.mozilla.org/projects/oak/rev/7addd5347f6a4619b24341ed55c97a016de361e6
Bug 1404480 - Manage hdiutil output when the volume name contains a space r?whimboo
https://hg.mozilla.org/mozilla-central/rev/e07d7bb8d948
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: