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

RESOLVED FIXED in Firefox 34

Status

defect
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

({intermittent-failure})

unspecified
mozilla36
All
Windows XP
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
+++ 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.
(Assignee)

Comment 1

5 years ago
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-
(Assignee)

Comment 4

5 years ago
(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.
(Assignee)

Updated

5 years ago
Summary: buildapp should not contain path. → buildapp should not have the difference of path separator between windows and others
(Assignee)

Comment 6

5 years ago
Posted 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.
(Assignee)

Comment 8

5 years ago
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+
(Assignee)

Comment 10

5 years ago
Thanks!
Attachment #8509439 - Attachment is obsolete: true
Attachment #8509492 - Flags: review?(mh+mozilla)
Attachment #8509492 - Flags: review?(mh+mozilla) → review+
(Assignee)

Updated

5 years ago
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
Last Resolved: 5 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

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.