Closed
Bug 584473
Opened 14 years ago
Closed 14 years ago
Making from the MSYS root directory is broken
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
WORKSFORME
People
(Reporter: khuey, Assigned: sfink)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
ted
:
review+
khuey
:
feedback+
benjamin
:
approval2.0-
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
I think the correct behavior here is to check (at configure or toplevel make time) for this case and print an error.
Reporter | ||
Comment 2•14 years ago
|
||
I agree. It's not worth making everybody pay the significant cost of fixing this (which would require multiple shell invocations in every directory).
Assignee | ||
Comment 4•14 years ago
|
||
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.
Reporter | ||
Comment 5•14 years ago
|
||
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•14 years ago
|
||
Should be possible, yeah, you can just call `pwd -W` in configure.
Assignee | ||
Comment 7•14 years ago
|
||
(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.
Assignee | ||
Comment 8•14 years ago
|
||
Attachment #467958 -
Flags: review?(ted.mielczarek)
Comment 9•14 years ago
|
||
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.
Attachment #467958 -
Flags: review?(ted.mielczarek) → review-
Assignee | ||
Comment 10•14 years ago
|
||
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•14 years ago
|
||
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.)
Assignee | ||
Comment 12•14 years ago
|
||
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.
Attachment #467958 -
Attachment is obsolete: true
Attachment #469284 -
Flags: review?(ted.mielczarek)
Comment 13•14 years ago
|
||
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.
Assignee | ||
Comment 14•14 years ago
|
||
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.
Reporter | ||
Comment 15•14 years ago
|
||
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.
Attachment #469284 -
Flags: feedback+
Reporter | ||
Comment 16•14 years ago
|
||
Feel free to take that f+ as r+.
Comment 17•14 years ago
|
||
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.
Attachment #469284 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 18•14 years ago
|
||
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.
Keywords: checkin-needed,
dev-doc-needed
Reporter | ||
Comment 19•14 years ago
|
||
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.
Attachment #469284 -
Flags: approval2.0?
Updated•14 years ago
|
Assignee: nobody → sphink
Keywords: checkin-needed
Updated•14 years ago
|
Attachment #469284 -
Flags: approval2.0? → approval2.0-
Comment 20•14 years ago
|
||
Unnecessary churn, please wait until after branching.
Comment 22•14 years ago
|
||
Is this still a bug after bug 584474?
Reporter | ||
Comment 23•14 years ago
|
||
This should just work now, reopen if that's not the case.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WORKSFORME
Comment 24•14 years ago
|
||
Updated documentation:
https://developer.mozilla.org/En/Developer_Guide/Build_Instructions/Windows_Prerequisites#Common_problems.2c_hints_and_restrictions
Keywords: dev-doc-needed → dev-doc-complete
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•