Closed Bug 1084997 Opened 6 years ago Closed 6 years ago

buildapp should not have the difference of path separator between windows and others

Categories

(Firefox Build System :: General, defect)

All
Windows XP
defect
Not set
normal

Tracking

(firefox34 fixed, firefox35 fixed, firefox36 fixed, firefox-esr31 unaffected)

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- fixed
firefox35 --- fixed
firefox36 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: hiro, Assigned: hiro)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #984194 +++

The path in buildapp is not useful. If buildapp does not contain path, it will be very useful to check comm-central build in xpcshell.ini in mozilla-central.
Attachment #8507389 - Flags: review?(mh+mozilla)
Comment on attachment 8507389 [details] [diff] [review]
do_not_include_path_in_buildapp.patch

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

::: python/mozbuild/mozbuild/mozinfo.py
@@ +53,5 @@
>      # Build app name
>      if 'MOZ_MULET' in substs and substs.get('MOZ_MULET') == "1":
>          d["buildapp"] = "mulet"
>      elif 'MOZ_BUILD_APP' in substs:
> +        d["buildapp"] = os.path.basename(substs["MOZ_BUILD_APP"])

I'm sure there are things relying on buildapp being "mobile/android" for fennec builds.
Attachment #8507389 - Flags: review?(mh+mozilla) → review-
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3)
> I don't see any of those currently, I only see one thing in the tree that
> this would break:
> http://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/tests/
> unit_ipc/xpcshell.ini#8

Yes, I just want to fix this case. This '../mail' is now broken on windows because the separator '/' is '\' on windows.
In these days there are lots of test failures in mozilla-central when those tests run from comm-central. So I am thinking such case will be increasing.
I think the patch is probably fine as-is, but alternately you could just do a .replace('\\', '/') and normalize everything to forward-slashes.
Summary: buildapp should not contain path. → buildapp should not have the difference of path separator between windows and others
Attached patch Replace '\' with '/' (obsolete) — Splinter Review
Thanks for the suggestion.
I did not think that such fix can be acceptable.
Attachment #8507389 - Attachment is obsolete: true
Attachment #8509429 - Flags: review?(ted)
Actually, this should be fixed way before that, MOZ_BUILD_APP is used in moz.build where things are already supposed to be using forward slashes. I'd argue this should be fixed in comm-central, so that it doesn't pass a backslash to mozilla/'s configure.
That trick has done in mozilla-central's configure.in.
A lots of backslashes are needed there...
I do not know actually why many backslashes are necessary there.
Attachment #8509429 - Attachment is obsolete: true
Attachment #8509429 - Flags: review?(ted)
Attachment #8509439 - Flags: review?(mh+mozilla)
Comment on attachment 8509439 [details] [diff] [review]
Replace '\' with '/' in MOZ_BUILD_APP

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

::: configure.in
@@ +3995,5 @@
>    fi
>    # Support comm-central.
>    if test -n "$EXTERNAL_SOURCE_DIR" ; then
>      MOZ_BUILD_APP="$EXTERNAL_SOURCE_DIR/$MOZ_BUILD_APP"
> +    MOZ_BUILD_APP=`${PYTHON} -c "import os.path; print(os.path.relpath(\"${MOZ_BUILD_APP}\", \"${srcdir}\").replace('\\\\\','/'))"`

Even better: ${PYTHON} -c "import mozpack.path as mozpath; print(mozpath.relpath(...))"

mozpath.relpath does the path normalization.
Attachment #8509439 - Flags: review?(mh+mozilla) → feedback+
Thanks!
Attachment #8509439 - Attachment is obsolete: true
Attachment #8509492 - Flags: review?(mh+mozilla)
Attachment #8509492 - Flags: review?(mh+mozilla) → review+
Assignee: nobody → hiikezoe
Keywords: checkin-needed
Can we please get a sanity check Try run? :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d26a42212f16
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Do we need this on any other branches as well?
Flags: needinfo?(hiikezoe)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #15)
> Do we need this on any other branches as well?

Yes please, at least aurora and beta. It may have also fixed our l10n repack issues. Do we need an approval request, or does it not matter as this is build-only?
Flags: needinfo?(hiikezoe)
Blocks: 1053761
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.