1077099 - Add GreBinD to easily differentiate between Contents/Resources (GreD) and Contents/MacOS on OSX

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Stephen A Pohl [:spohl]]

Due to the v2 signature changes on OSX (bug 1047584), we had to move all non-code files from Contents/MacOS to Contents/Resources in the app bundle. At the same time we changed the meaning of GreD and made it point to Contents/Resources in the app bundle. Unfortunately, this left us without an easy way to access Contents/MacOS.

The idea is to introduce GreBinD which points to Contents/MacOS on OSX and will be equivalent to GreD on all other platforms.

Comment 1

[Robert Strong (they/them - no direct email)]

If you haven't started I'll try to get to this tonight

Comment 2

[Stephen A Pohl [:spohl]]

Patch is well underway, thanks!

Comment 3

[Robert Strong (they/them - no direct email)]

I just realized that I'm not certain if bsmedberg prefers to have this in nsDirectoryService.cpp or in nsXREDirProvider.cpp and XPCShellImpl.cpp (for xpcshell tests).

Comment 4

[Robert Strong (they/them - no direct email)]

Benjamin, do you prefer to have this in nsDirectoryService.cpp or in nsXREDirProvider.cpp and XPCShellImpl.cpp (for xpcshell tests).

Comment 5

[Stephen A Pohl [:spohl]]

Attachment: Patch (wip)

Figured it can't hurt to post my work in progress. This merely compiles but I have yet to do any testing, or change any callers to use GreBinD.

Comment 6

[Stephen A Pohl [:spohl]]

Attachment: Patch (wip)

qrefresh'd for latest changes.

Comment 7

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #4)
> Benjamin, do you prefer to have this in nsDirectoryService.cpp or in
> nsXREDirProvider.cpp and XPCShellImpl.cpp (for xpcshell tests).
Meant to write
in nsDirectoryService.cpp, nsXREDirProvider.cpp, and XPCShellImpl.cpp or just in nsXREDirProvider.cpp and XPCShellImpl.cpp.

Clearing feedback request since I am going to test this out and if all is good request review.

Comment 8

[Robert Strong (they/them - no direct email)]

Comment on attachment 8499300 [details] [diff] [review]
Patch (wip)

I verified this works on both Windows and Mac OS X in both Firefox and xpcshell so requesting review.

Comment 9

[Robert Strong (they/them - no direct email)]

Try was locked up so I pushed to oak
https://hg.mozilla.org/projects/oak/rev/763c82f73410

Comment 10

[Robert Strong (they/them - no direct email)]

Test results can be seen here
https://tbpl.mozilla.org/?tree=Oak&rev=7d0ccd91e8db

Comment 11

[Robert Strong (they/them - no direct email)]

Comment on attachment 8499300 [details] [diff] [review]
Patch (wip)

This works beautifully for tests, etc. but it breaks during make package which uses the -g command line arg.

Comment 12

[Stephen A Pohl [:spohl]]

Attachment: Patch

Right, I hadn't tested the last patch yet. This patch seems to work for me now. Robert, before I request review, could you please confirm that this works for you as well? Thanks!

Comment 13

[Stephen A Pohl [:spohl]]

Attachment: Patch

Oops, forgot to clean up XPCShellDirProvider::GetFile before uploading.

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 8499587 [details] [diff] [review]
Patch

Works for me!

Comment 15

[Stephen A Pohl [:spohl]]

Comment on attachment 8499587 [details] [diff] [review]
Patch

Great, thanks Robert!

Comment 16

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0de83f6ba398
https://hg.mozilla.org/projects/oak/rev/4cf665f93be3

Comment 17

[Robert Strong (they/them - no direct email)]

Jorge, heads up about a new dir servive provider key to use when getting the binary directory. The key is GreBinD and will be the same directory as GreD on all platforms but Mac OS X. For Mac OS X GreD points to Contents/Resources/ and GreBinD points to Contents/MacOS/

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0de83f6ba398

Comment 19

[Nicholas Nethercote [inactive]]

It appears that |xrePath| is now

> $OBJDIR/dist/NightlyDebug.app/Contents/Resources/

but glandium says it should be 

> $OBJDIR/dist/NightlyDebug.app/Contents/MacOS/

Comment 20

[Stephen A Pohl [:spohl]]

(In reply to Nicholas Nethercote [:njn] from comment #19)
> It appears that |xrePath| is now
> 
> > $OBJDIR/dist/NightlyDebug.app/Contents/Resources/
> 
> but glandium says it should be 
> 
> > $OBJDIR/dist/NightlyDebug.app/Contents/MacOS/

|xrePath| has changed to Contents/Resources. See bug 1050944 comment 28. As of the landing of this bug here, you may query GreBinD to get access to Contents/MacOS directly.

Comment 21

[Eric Shepherd [:sheppy]]

See comments 17 and 20 for documentation information. This is related to bug 1050944.

Comment 22

[Eric Shepherd [:sheppy]]

Docs to update:

https://developer.mozilla.org/en-US/docs/Using_nsIDirectoryService

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIDirectoryServiceProvider/getFile

And while we're at it, https://developer.mozilla.org/en-US/docs/FileGuide/Accessing_Files should be updated so that its mention that there are lots of directories you can look up should be crosslinked to the list provided in the getFile article.

Comment 23

[Stephen A Pohl [:spohl]]

Attachment: Followup

While working on bug 1077282 I noticed that there will need to be a followup to this bug. Marking as 'wip' while I'm working through the remaining test failures from the cleanup in bug 1077282, as there may need to be more client-side changes.

Comment 24

[Stephen A Pohl [:spohl]]

Comment on attachment 8501179 [details] [diff] [review]
Followup

Previously, |nsXREDirProvider| had to be queried for NS_GRE_BIN_DIR at least once before |nsXREDirProvider::GetGREBinDir| would return the correct GREBinD (i.e. non-null). I moved the initialization of |mGREBinDir| to |nsXREDirProvider::Initialize|, where it probably should have been all along...

Also made XPCShellDirProvider GreBinD-aware.

Comment 25

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb27bd0c0084

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fb27bd0c0084

Comment 27

[Robert Strong (they/them - no direct email)]

Landed on aurora in the Mac V2 signing combined patch in bug 1047584