Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[android-toolchain] Remove use of $(MAKEFLAGS) #603

Merged
merged 1 commit into from
May 25, 2017

Conversation

jonpryor
Copy link
Member

An attempt to bump external/mono/ resulted in a
build failure on a macOS machine:

Executing: make MSBUILD_ARGS=/p:AutoProvision=True/ /p:AutoProvisionUsesSudo=True V=1 MXE_TARGETS="i686-w64-mingw32.static" gcc cmake zlib pt hreads dlfcn-win32 mman-win32 PREFIX="/Users/builder/android-toolchain/mxe"
make[1]: *** No rule to make target `/p:AutoProvisionUsesSudo=True'.  Stop.

(This happens on a macOS machine that hasn't previously
built+installed MXE which needs MXE in order to generate Windows
binaries, so auto-builds MXE...)

This error is due to the interraction of three separate choices:

  1. Jenkins is running:

    make prepare jenkins V=1 MSBUILD_ARGS="/p:AutoProvision=True /p:AutoProvisionUsesSudo=True"
    
  2. xbuild replaces \ with /.

  3. The _CreateMxeToolchains target uses make $(MAKEFLAGS).

Background: what is $(MAKEFLAGS)? $(MAKEFLAGS) is set by
make(1) and contains the command-line flags to make.
It's value is shown elsewhere in the log file:

MAKEFLAGS = MSBUILD_ARGS=/p:AutoProvision=True\ /p:AutoProvisionUsesSudo=True V=1

Note that $(MAKEFLAGS) contains a backslash: this is because of the
original command, which provides a space-containing value for
$(MSBUILD_ARGS):

MSBUILD_ARGS="/p:AutoProvision=True /p:AutoProvisionUsesSudo=True"

Instead of preserving the quotes, make instead replaces (space)
with \ (backslash-space), which is perfectly valid:

# MAKEFLAGS set "as if" we executed:
make prepare jenkins V=1 MSBUILD_ARGS=/p:AutoProvision=True\ /p:AutoProvisionUsesSudo=True

The problem is when xbuild enters the picture: xbuild can't
differentiate between paths and...anything else, so for simplicity and
sanity it always replaces \ with the directory-separtor-char,
which is / on macOS/Linux.

Unfortunately, this completely changes the semantics of the embedded
MSBUILD_ARGS value within $(MAKEFLAGS): replacing \ with /
causes the /p:AutoProvisionUsesSudo=True to be treated as target by
make, and that target doesn't exist.

In theory, xbuild could be fixed to address this. In practice,
xbuild isn't getting any future work. msbuild is getting more
work, but this "corner case" is likely sufficiently complicated that
it might not ever get fixed.

Thus, the simple fix: Don't Do That™: remove use of $(MAKEFLAGS).
It isn't needed in this invocation.

An attempt to [bump `external/mono/`][0] resulted in a
[build failure on a macOS machine][1]:

	Executing: make MSBUILD_ARGS=/p:AutoProvision=True/ /p:AutoProvisionUsesSudo=True V=1 MXE_TARGETS="i686-w64-mingw32.static" gcc cmake zlib pt hreads dlfcn-win32 mman-win32 PREFIX="/Users/builder/android-toolchain/mxe"
	make[1]: *** No rule to make target `/p:AutoProvisionUsesSudo=True'.  Stop.

(This happens on a macOS machine that hasn't previously
built+installed MXE which needs MXE in order to generate Windows
binaries, so auto-builds MXE...)

This error is due to the interraction of three separate choices:

 1. Jenkins is running:

        make prepare jenkins V=1 MSBUILD_ARGS="/p:AutoProvision=True /p:AutoProvisionUsesSudo=True"

 2. xbuild replaces `\` with `/`.

 3. The `_CreateMxeToolchains` target uses `make $(MAKEFLAGS)`.

Background: what is `$(MAKEFLAGS)`? `$(MAKEFLAGS)` is set by
**make**(1) and [contains the command-line flags to `make`][2].
It's value is shown elsewhere in the log file:

	MAKEFLAGS = MSBUILD_ARGS=/p:AutoProvision=True\ /p:AutoProvisionUsesSudo=True V=1

Note that `$(MAKEFLAGS)` contains a backslash: this is because of the
original command, which provides a space-containing value for
`$(MSBUILD_ARGS)`:

	MSBUILD_ARGS="/p:AutoProvision=True /p:AutoProvisionUsesSudo=True"

Instead of preserving the quotes, `make` instead replaces ` ` (space)
with `\ ` (backslash-space), which is perfectly valid:

	# MAKEFLAGS set "as if" we executed:
	make prepare jenkins V=1 MSBUILD_ARGS=/p:AutoProvision=True\ /p:AutoProvisionUsesSudo=True

The problem is when `xbuild` enters the picture: xbuild can't
differentiate between paths and...anything else, so for simplicity and
sanity it *always* replaces `\` with the directory-separtor-char,
which is `/` on macOS/Linux.

Unfortunately, this *completely* changes the semantics of the embedded
`MSBUILD_ARGS` value within `$(MAKEFLAGS)`: replacing `\ ` with `/ `
causes the `/p:AutoProvisionUsesSudo=True` to be treated as target by
`make`, and that target doesn't exist.

In theory, `xbuild` could be fixed to address this. In practice,
`xbuild` isn't getting any future work. `msbuild` *is* getting more
work, but this "corner case" is likely sufficiently complicated that
it might not ever get fixed.

Thus, the simple fix: Don't Do That™: remove use of `$(MAKEFLAGS)`.
It isn't needed in this invocation.

[0]: dotnet#600
[1]: https://jenkins.mono-project.com/job/xamarin-android-pr-builder/955
[2]: https://www.gnu.org/software/autoconf/manual/autoconf-2.63/html_node/The-Make-Macro-MAKEFLAGS.html
@jonpryor jonpryor requested a review from grendello May 24, 2017 20:44
@grendello grendello merged commit 9d93345 into dotnet:master May 25, 2017
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Mar 20, 2020
Changes: dotnet/java-interop@cedf4d0...f7ea4ed

  * dotnet/java-interop@f7ea4ed: [Xamarin.Android.Tools.Bytecode-Tests] Remove obsolete files (dotnet#603)
  * dotnet/java-interop@e070ce3: [build] Remove <Imports/> now covered by Directory.Build.props (dotnet#607)
  * dotnet/java-interop@e7c5f54: [generator] Add netcoreapp3.1 support (dotnet#606)
  * dotnet/java-interop@95f698b: [build] Build additional `$(TargetFramework)` values. (dotnet#605)
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Mar 20, 2020
Changes: dotnet/java-interop@cedf4d0...1a086ff

  * dotnet/java-interop@1a086ff: [Java.Interop.Tools.JavaCallableWrappers] Fix IsValidIdentifier() (dotnet#610)
  * dotnet/java-interop@f7ea4ed: [Xamarin.Android.Tools.Bytecode-Tests] Remove obsolete files (dotnet#603)
  * dotnet/java-interop@e070ce3: [build] Remove <Imports/> now covered by Directory.Build.props (dotnet#607)
  * dotnet/java-interop@e7c5f54: [generator] Add netcoreapp3.1 support (dotnet#606)
  * dotnet/java-interop@95f698b: [build] Build additional `$(TargetFramework)` values. (dotnet#605)
jonpryor added a commit that referenced this pull request Mar 21, 2020
Changes: dotnet/java-interop@cedf4d0...1a086ff

  * dotnet/java-interop@1a086ff: [Java.Interop.Tools.JavaCallableWrappers] Fix IsValidIdentifier() (#610)
  * dotnet/java-interop@f7ea4ed: [Xamarin.Android.Tools.Bytecode-Tests] Remove obsolete files (#603)
  * dotnet/java-interop@e070ce3: [build] Remove <Imports/> now covered by Directory.Build.props (#607)
  * dotnet/java-interop@e7c5f54: [generator] Add netcoreapp3.1 support (#606)
  * dotnet/java-interop@95f698b: [build] Build additional `$(TargetFramework)` values. (#605)
@github-actions github-actions bot locked and limited conversation to collaborators Feb 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants