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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

+++ 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.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: do_not_include_path_in_buildapp.patch

Comment 2

[Mike Hommey [:glandium]]

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.

Comment 3

[(not currently active) Ted Mielczarek]

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

All dxr search results:
http://dxr.mozilla.org/mozilla-central/search?q=buildapp&offset=100

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(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.

Comment 5

[(not currently active) Ted Mielczarek]

I think the patch is probably fine as-is, but alternately you could just do a .replace('\\', '/') and normalize everything to forward-slashes.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Replace '\' with '/'

Thanks for the suggestion.
I did not think that such fix can be acceptable.

Comment 7

[Mike Hommey [:glandium]]

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.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Replace '\' with '/' in MOZ_BUILD_APP

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.

Comment 9

[Mike Hommey [:glandium]]

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.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Replace '\' with '/' using mozpack.path.relpath

Thanks!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Can we please get a sanity check Try run? :)

Comment 12

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d0dbf487f441

Comment 13

[Carsten Book [:Tomcat]]

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

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Do we need this on any other branches as well?

Comment 16

[Mark Banner (:standard8)]

(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?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/318d41dfecf0
https://hg.mozilla.org/releases/mozilla-beta/rev/4197f5318fd8