Skip to content

Fix Dub package when cross-compiling#9275

Merged
dlang-bot merged 2 commits intodlang:masterfrom
jacob-carlborg:dub-package-cross-compile
Jan 21, 2019
Merged

Fix Dub package when cross-compiling#9275
dlang-bot merged 2 commits intodlang:masterfrom
jacob-carlborg:dub-package-cross-compile

Conversation

@jacob-carlborg
Copy link
Copy Markdown
Contributor

There are two commits, I recommend reviewing them separately.

@dlang-bot
Copy link
Copy Markdown
Contributor

Thanks for your pull request and interest in making D better, @jacob-carlborg! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#9275"

@WalterBright
Copy link
Copy Markdown
Member

There are two commits, I recommend reviewing them separately.

Please do them as 2 PRs, then.

@jacob-carlborg
Copy link
Copy Markdown
Contributor Author

I don't see the problem of looking at the individual commits.

@thewilsonator
Copy link
Copy Markdown
Contributor

DAutoTest

Running pre-generate commands for dmd:lexer...
/bin/sh: line 2: : command not found
Command failed with exit code 127: 
    "${DUB_EXE}" \
    --arch=${DUB_ARCH} \
    --single "${DUB_PACKAGE_DIR}config.d" \
    -- "${DUB_PACKAGE_DIR}generated/dub" \
    "${DUB_PACKAGE_DIR}VERSION" \
    /etc

Say what?

@jacob-carlborg
Copy link
Copy Markdown
Contributor Author

Say what?

Hmm, I didn't know that DAutoTest was building the Dub package. But for Semaphore I instead get:

/bin/sh: 2: : Permission denied
Command failed with exit code 127: 
    "${DUB_EXE}" \
    --arch=${DUB_ARCH} \
    --single "${DUB_PACKAGE_DIR}config.d" \
    -- "${DUB_PACKAGE_DIR}generated/dub" \
    "${DUB_PACKAGE_DIR}VERSION" \
    /etc

@jacob-carlborg
Copy link
Copy Markdown
Contributor Author

Say what?

Turns out that the $DUB_EXE variable was added in the release shipped with DMD 2.084.0 [1] (and the changelog is wrong). DAutoTest is using Dub from DMD 2.083.0. Can we update that?

[1] https://dlang.org/changelog/2.084.0.html#dubEnvVar

@jacob-carlborg jacob-carlborg added the Review:Blocking Other Work review and pulling should be a priority label Jan 20, 2019
@jacob-carlborg
Copy link
Copy Markdown
Contributor Author

BTW, this blocks: #8528.

@thewilsonator
Copy link
Copy Markdown
Contributor

Can we update that?

cc @CyberShadow

@CyberShadow
Copy link
Copy Markdown
Member

Can we update that?

I think it's coming from here:
https://github.com/dlang/dlang.org/blob/master/posix.mak#L183

@jacob-carlborg
Copy link
Copy Markdown
Contributor Author

@CyberShadow
Copy link
Copy Markdown
Member

CyberShadow commented Jan 20, 2019

Unless it is known for a fact that $DUB_EXE will not match $(which dub), I suggest using "$${DUB_EXE:-dub}" to allow this change to work with older versions of Dub.

@jacob-carlborg
Copy link
Copy Markdown
Contributor Author

Unless it is known for a fact that $DUB_EXE will not match $(which dub)

If dub is not in the PATH then which dub will not work. But $DUB_EXE will, it uses thisExePath.

@CyberShadow
Copy link
Copy Markdown
Member

If dub is not in the PATH then which dub will not work.

That's fine. If that's your only concern, I recommend that you apply the change I suggested above.

@jacob-carlborg
Copy link
Copy Markdown
Contributor Author

Any equivalent for Windows?

@CyberShadow
Copy link
Copy Markdown
Member

Not in its variable substitution syntax, as far as I know, so only as a separate command. Not updating the Windows command only means that Windows users will have no choice but to update to 2.084, which doesn't seem like a problem.

@jacob-carlborg
Copy link
Copy Markdown
Contributor Author

I got this error following your advice:

/bin/sh: line 2: 29755{DUB_EXE:-dub}: command not found

@CyberShadow
Copy link
Copy Markdown
Member

CyberShadow commented Jan 20, 2019

And $${DUB_EXE} (with two $) works? Is that some special Dub syntax? There should probably be only one $, with :-dub or without.

@jacob-carlborg
Copy link
Copy Markdown
Contributor Author

And $${DUB_EXE} (with two $$) works? Is that some special Dub syntax?

Yes, it's special Dub syntax. Some variables will be substituted before the command is run.

@CyberShadow
Copy link
Copy Markdown
Member

CyberShadow commented Jan 20, 2019

I can't find the implementation for parsing that syntax in Dub's source code. It looks like it's just using spawnShell, and everything points to that those variables are expanded by the shell, not Dub. Are you sure there are supposed to be two $?

Edit: Found something, looking into it.

@jacob-carlborg
Copy link
Copy Markdown
Contributor Author

jacob-carlborg commented Jan 20, 2019

I can't find the implementation for parsing that syntax in Dub's source code. It looks like it's just using spawnShell, and everything points to that those variables are expanded by the shell, not Dub.

It must be doing that before calling spawnShell.

Are you sure there are supposed to be two $?

Yes, otherwise I get Invalid variable: DUB_EXE. See https://dub.pm/package-format-sdl.html#environment-variables: "To denote a literal dollar sign, use $$.".

@CyberShadow
Copy link
Copy Markdown
Member

Oh. Looks like it's a Dub bug. Look at this function:

https://github.com/dlang/dub/blob/927c5e6c248f7e2098a303fd475decfd8d8cca0a/source/dub/project.d#L1197-L1211

Instead of looking for $ (minding escapes), it uses a regular expression to match what it thinks shell variable use looks like. The code looks wrong and overcomplicated, I guess I'll work on a fix.

Never mind my suggestion I guess...

@jacob-carlborg
Copy link
Copy Markdown
Contributor Author

The issue comes from the fact that Dub uses the same variable syntax as Bash. On Windows [1] it's not a problem because the shell on Windows uses % instead of $.

[1] https://github.com/dlang/dmd/pull/9275/files#diff-406ece895a5b70a272cd76a2ef902836R43

@CyberShadow
Copy link
Copy Markdown
Member

Yes, but that doesn't change what I wrote above. Its escaping logic is wrong.

@jacob-carlborg
Copy link
Copy Markdown
Contributor Author

Ok.

@CyberShadow
Copy link
Copy Markdown
Member

I guess I'll work on a fix.

Here it is: dlang/dub#1641

@jacob-carlborg jacob-carlborg force-pushed the dub-package-cross-compile branch from c378bde to 5d0d648 Compare January 20, 2019 21:22
@PetarKirov
Copy link
Copy Markdown
Member

@jacob-carlborg In order for the tests to pass on SemaphoreCI, replace

dmd/semaphoreci.sh

Lines 39 to 48 in 54a2a5a

# FIXME: v2.082.0 has a broken DUB which fails the CI
# Remove this when a fixed v2.082.1 is released
# See https://github.com/dlang/dub/issues/1551
if [ "$DMD" == "dmd" ]; then
install_d "dmd-2.081.2"
elif [ "$DMD" == "ldc" ]; then
install_d "ldc-1.11.0"
else
install_d "$DMD"
fi

with:

install_d "$DMD"

@jacob-carlborg jacob-carlborg force-pushed the dub-package-cross-compile branch 2 times, most recently from 7794aea to dff9e49 Compare January 21, 2019 07:28
The pre generate commands were using the native platform instead of
target platform.
@jacob-carlborg jacob-carlborg force-pushed the dub-package-cross-compile branch from 0b3a621 to ca4f0d7 Compare January 21, 2019 11:38
@jacob-carlborg
Copy link
Copy Markdown
Contributor Author

@thewilsonator @ZombineDev all tests are passing, finally 😃.

@thewilsonator
Copy link
Copy Markdown
Contributor

Nice I take it it is goo to go?

@jacob-carlborg
Copy link
Copy Markdown
Contributor Author

Nice I take it it is goo to go?

Yes. I had to download Dub separately on Semaphore when using LDC 1.11.0. Because the version of Dub shipped with LDC 1.11.0 is too old and doesn't support the DUB_EXE environment variable. Using a later version of LDC cause some linker error.

@thewilsonator
Copy link
Copy Markdown
Contributor

Can you report the linker error please?

@jacob-carlborg
Copy link
Copy Markdown
Contributor Author

Sure.

@thewilsonator
Copy link
Copy Markdown
Contributor

Thanks.

@dlang-bot dlang-bot merged commit ba961d6 into dlang:master Jan 21, 2019
@jacob-carlborg jacob-carlborg deleted the dub-package-cross-compile branch January 21, 2019 13:08
@jacob-carlborg
Copy link
Copy Markdown
Contributor Author

Has already been reported: ldc-developers/ldc#2954.

@jacob-carlborg jacob-carlborg removed the Review:Blocking Other Work review and pulling should be a priority label Jan 21, 2019
kinke added a commit to kinke/dmd that referenced this pull request Jul 26, 2024
The build of the lexer dub subpackage requires running and building
a little config.d tool as custom pregenerate command.

Forwarding the target architecture via `--arch=$DUB_ARCH` to the
nested dub build of config.d was added in dlang#9275, to fix cross-
compilation, but in reality broke it.

Not specifying the architecture explicitly for the little helper
build lets the compiler pick the default one, the host's native
platform in practice, which is guaranteed to be runnable on that
compiling **host**, independent from the **target** platform for
the main dub build. Suppose one cross-compiles the dmd:lexer
subpackage from x86_64 to AArch64 - an AArch64 config.d executable
will hardly run on the x86_64 host, and won't be able to generate
the `VERSION` and `SYSCONFDIR.imp` string-import files as pre-
requisite of the build.

Side note: using little separately-built .d tools/scripts as build
helpers for autogenerating little VERSION files etc. is IMO bad
practice - when cross-compiling, you require a D compiler that can
a) cross-compile, and b) build successfully for the native platform
too. Not sure this approach will e.g. ever work with GDC, where you
have different toolchains for each host->target combination.
thewilsonator pushed a commit that referenced this pull request Jul 28, 2024
The build of the lexer dub subpackage requires running and building
a little config.d tool as custom pregenerate command.

Forwarding the target architecture via `--arch=$DUB_ARCH` to the
nested dub build of config.d was added in #9275, to fix cross-
compilation, but in reality broke it.

Not specifying the architecture explicitly for the little helper
build lets the compiler pick the default one, the host's native
platform in practice, which is guaranteed to be runnable on that
compiling **host**, independent from the **target** platform for
the main dub build. Suppose one cross-compiles the dmd:lexer
subpackage from x86_64 to AArch64 - an AArch64 config.d executable
will hardly run on the x86_64 host, and won't be able to generate
the `VERSION` and `SYSCONFDIR.imp` string-import files as pre-
requisite of the build.

Side note: using little separately-built .d tools/scripts as build
helpers for autogenerating little VERSION files etc. is IMO bad
practice - when cross-compiling, you require a D compiler that can
a) cross-compile, and b) build successfully for the native platform
too. Not sure this approach will e.g. ever work with GDC, where you
have different toolchains for each host->target combination.
thewilsonator pushed a commit to thewilsonator/dmd that referenced this pull request Oct 7, 2024
The build of the lexer dub subpackage requires running and building
a little config.d tool as custom pregenerate command.

Forwarding the target architecture via `--arch=$DUB_ARCH` to the
nested dub build of config.d was added in dlang#9275, to fix cross-
compilation, but in reality broke it.

Not specifying the architecture explicitly for the little helper
build lets the compiler pick the default one, the host's native
platform in practice, which is guaranteed to be runnable on that
compiling **host**, independent from the **target** platform for
the main dub build. Suppose one cross-compiles the dmd:lexer
subpackage from x86_64 to AArch64 - an AArch64 config.d executable
will hardly run on the x86_64 host, and won't be able to generate
the `VERSION` and `SYSCONFDIR.imp` string-import files as pre-
requisite of the build.

Side note: using little separately-built .d tools/scripts as build
helpers for autogenerating little VERSION files etc. is IMO bad
practice - when cross-compiling, you require a D compiler that can
a) cross-compile, and b) build successfully for the native platform
too. Not sure this approach will e.g. ever work with GDC, where you
have different toolchains for each host->target combination.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants