584473 - Making from the MSYS root directory is broken

Status: RESOLVED WORKSFORME

(Firefox Build System :: General, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Since Bug 522770 landed making from the MSYS root directory has been broken.  That is, a path like /foo/bar/ will be expanded to foo:\bar in the fakelibs stuff which will cause the linker to die.

Comment 1

[Benjamin Smedberg]

I think the correct behavior here is to check (at configure or toplevel make time) for this case and print an error.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I agree.  It's not worth making everybody pay the significant cost of fixing this (which would require multiple shell invocations in every directory).

Comment 4

[Steve Fink [:sfink] [:s:]]

I documented this limitation in https://developer.mozilla.org/En/Developer_Guide/Build_Instructions/Windows_Prerequisites

I don't think it would require shell invocations per-directory, though. Everything is under OBJDIR, no? So that portion could trigger the shell invocations and then be stored, and all the directories would append onto that.

I took a quick stab at it, but was not immediately successful and I agree that it's probably not important enough to spend much time on.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Can we set the objdir's winpath as a configure variable and then use that?  We'd only have to pay that cost once then.

Comment 6

[(not currently active) Ted Mielczarek]

Should be possible, yeah, you can just call `pwd -W` in configure.

Comment 7

[Steve Fink [:sfink] [:s:]]

(In reply to comment #5)
> Can we set the objdir's winpath as a configure variable and then use that? 
> We'd only have to pay that cost once then.

I don't understand OBJDIR vs MOZ_OBJDIR, but neither of them seemed to be set where I needed this. But I think what you're suggesting already exists, and is called MOZ_BUILD_ROOT.

I will attach a patch that worked for me (when I still had stuff under there). It adds a check to error out if you attempt to core_winabspath anything outside of MOZ_BUILD_ROOT.

Comment 8

[Steve Fink [:sfink] [:s:]]

Attachment: Fix core_winabspath to work from anywhere

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 467958 [details] [diff] [review]
Fix core_winabspath to work from anywhere

No, we're not taking something that runs a shell command every time we need to call core_winabspath.

Comment 10

[Steve Fink [:sfink] [:s:]]

It doesn't, unless I've managed to severely confuse myself.

The patch uses a single shell call to cache the translated form of MOZ_BUILD_ROOT *once per compile*. Calls to core_winabspath use the cached translation and append the trailing portion of the path to it, using only internal make commands (no shell commands.)

I apologize if my description left you with the wrong impression.

Comment 11

[(not currently active) Ted Mielczarek]

Our build system uses recursive make, so we run make in every single directory with a Makefile. Every single Makefile includes config/config.mk, so every single call site of core_winabspath will have a shell invocation. (In addition, the way GNU Make variables are evaluated, unless you assign with :=, the variable will be reevaluated for every single reference, so you could wind up with multiple shell invocations per Makefile.)

Comment 12

[Steve Fink [:sfink] [:s:]]

Attachment: Fix windows path handling

Ouch. Ouch! Excuse me while I go hide my head in shame...

Ok, I'm back. With more problems. I moved away from putting things under the msys root by sharing a W: directory between my VM and host, but I still saw build failures. It turned out to be a separate, more severe issue. Let me know if I should open a separate bug -- these are related, so I'm being lazy.

It turns out that the definition of win_srcdir in config/config.mk has the reverse bug:

  $(subst $(topsrcdir),$(WIN_TOP_SRC),$(srcdir))

If you are intentionally avoiding the msys root, as I was, and putting your code into eg W:\, then this boils down to $(subst /w,W:/,$(srcdir)). But $(subst...) replaces *all* instances, so if srcdir happens to be, say, /w/obj-win/modules/plugin/sdk/samples/basic/windows, it will mangle it into W://obj-win/modules/plugin/sdk/samples/basicW:/indows.

So currently, things won't work under msys if your source tree is either under the msys root, or it's directly under a drive letter that happens to match the first letter of a subdirectory.

The attached patch fixes both. Let me know if you'd prefer I split it into the two individual issues.

Comment 13

[(not currently active) Ted Mielczarek]

Sorry, I keep meaning to review this, and every time I look at the patch I realize I'll have to actually apply it and poke at it to understand it. I'll try to get to it today.

Comment 14

[Steve Fink [:sfink] [:s:]]

The main change, used for both fixes included in this patch, is

-win_srcdir	:= $(subst $(topsrcdir),$(WIN_TOP_SRC),$(srcdir))
+win_srcdir	:= $(patsubst $(topsrcdir)%,$(WIN_TOP_SRC)%,$(srcdir))

i.e., using

  $(patsubst PREFIX%,REPLACEMENT%,PATH)
instead of
  $(subst PREFIX,REPLACEMENT,PATH)

to replace a singe occurrence of PREFIX with REPLACEMENT in PATH instead of all of them. (The fakewinpath stuff is just that with an added error check to be sure your paths are properly rooted.)

well, that and some cargo-culted cygwin stuff that hopefully won't break cygwin any more than it already is.

But I agree that this is not eye-inspectable (and that testing anything on Windows is a pain.) You could always pass it over to khuey to further the "distract khuey from his midterms so he drops out and joins Mozilla sooner" campaign.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 469284 [details] [diff] [review]
Fix windows path handling

This version is an order of magnitude easier to follow than the version I wrote at 3 AM.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Feel free to take that f+ as r+.

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 469284 [details] [diff] [review]
Fix windows path handling

Ok. I read this through enough times and convinced myself it was ok.

Incidentally, we should ditch all the cygwin stuff, but that's already filed as bug 462361.

Comment 18

[Steve Fink [:sfink] [:s:]]

Tagging as dev-doc-needed as a reminder to myself to remove the restriction from https://developer.mozilla.org/En/Developer_Guide/Build_Instructions/Windows_Prerequisites when this lands.

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 469284 [details] [diff] [review]
Fix windows path handling

This needs a+ too.

We should take this fix because it both fixes the problem and makes this piece of code not awful.

Comment 20

[Benjamin Smedberg]

Unnecessary churn, please wait until after branching.

Comment 22

[(not currently active) Ted Mielczarek]

Is this still a bug after bug 584474?

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This should just work now, reopen if that's not the case.

Comment 24

[Eric Shepherd [:sheppy]]

Updated documentation:

https://developer.mozilla.org/En/Developer_Guide/Build_Instructions/Windows_Prerequisites#Common_problems.2c_hints_and_restrictions