Revert "Revert "Add mingwW64-llvm cross-system."" #173498
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Waiting on next staging cycle to move forward. This reverts commit c911240.
shlevy
requested review from
matthewbauer,
cdepillabout,
sternenseemann,
maralorn,
expipiplus1,
Ericson2314,
alyssais and
nbp
as code owners
ofborg
bot
added
10.rebuild-darwin-stdenv
|
@sternenseemann Any sense of when you'll get a chance to review? |
There was a problem hiding this comment.
Overall looks okay to me, but I can't really comment on the libtool patches and haven't tested it.
Doing things for LLVM >= 13 as long as it includes git is okay, may be nice to add it for 11 and 12 as well so it hits our default versions may be nice as well, but not a requirement.
| in '' | ||
| export mtype=${mtype} | ||
| substituteAll ${./add-lld-ldflags-before.sh} add-local-ldflags-before.sh | ||
| cat add-local-ldflags-before.sh >> $out/nix-support/add-local-ldflags-before.sh |
There was a problem hiding this comment.
Can you change the other writes to $out/nix-support/add-local-ldflags-before.sh to append as well? I'm a bit concerned that somebody starts overwriting additions accidentally by reordering code in here.
| preferLocalBuild = true; | ||
| passthru = { | ||
| isLld = true; | ||
| targetPrefix = prefix; |
There was a problem hiding this comment.
Why don't the other LLVM versions have a targetPrefix passthru now?
| '' + lib.optionalString stdenv.targetPlatform.isWindows '' | ||
| ln -s ${llvm}/bin/llvm-windres $out/bin/${prefix}windres | ||
| ln -s ${llvm}/bin/llvm-dlltool $out/bin/${prefix}dlltool | ||
| '') |
There was a problem hiding this comment.
Is this relevant to other LLVM versions as well? Note that unless you add any changes to llvmPackages 14 and llvmPackages_git, it'll likely get lost in the future.
| ./0012-Prefer-response-files-over-linker-scripts-for-mingw-.patch | ||
| ./0013-Allow-statically-linking-compiler-support-libraries-.patch | ||
| ./0014-Support-llvm-objdump-f-output.patch | ||
| ]; |
There was a problem hiding this comment.
Can't say anything w.r.t. these patches. Maybe we can pester @aakropotkin about this?
| @@ -45,6 +47,8 @@ stdenv.mkDerivation rec { | |||
|
|
|||
| checkInputs = [ dejagnu ]; | |||
|
|
|||
| nativeBuildInputs = lib.optional stdenv.hostPlatform.isWindows autoreconfHook; | |||
There was a problem hiding this comment.
Can you add a comment why this is needed? I.e. is this a structural problem or will this fix itself in a future libffi release?
| @@ -29,7 +29,7 @@ let self = stdenv.mkDerivation rec { | |||
| passthru.static = self.out; | |||
|
|
|||
| depsBuildBuild = [ buildPackages.stdenv.cc ]; | |||
| nativeBuildInputs = [ m4 ]; | |||
| nativeBuildInputs = [ m4 ] ++ lib.optional stdenv.hostPlatform.isWindows autoreconfHook; | |||
There was a problem hiding this comment.
Can you add a comment why this is needed? I.e. is this a structural problem or will this fix itself in a future gmp release?
| "-DLIBCXX_ENABLE_STATIC_ABI_LIBRARY=TRUE" | ||
| "-DLIBCXX_CXX_ABI_LIBRARY_PATH=${libcxxabi}/lib" | ||
| ] ++ lib.optional (!headersOnly && libcxxabi.useLLVMUnwinder) | ||
| "-DLIBCXXABI_USE_LLVM_UNWINDER=ON"; |
There was a problem hiding this comment.
Still confused about this one…
|
|
||
| stdenv.mkDerivation rec { | ||
| enableShared' = enableShared && !semi-static; |
There was a problem hiding this comment.
Maybe onlyEnableShared is clearer?
|
I reviewed those patches recently for `libtool` and rejected them because
they improperly handle flag specs, and will cause static linkage to become
high risk for ABI conflicts.
If you can be certain that those patches will only be applied to individual
derivations I think it lowers the risk, but be on the lookout for ABI
issues especially in C++.
…On Tue, May 31, 2022, 5:25 AM sternenseemann ***@***.***> wrote:
***@***.**** commented on this pull request.
Overall looks okay to me, but I can't really comment on the libtool
patches and haven't tested it.
Doing things for LLVM >= 13 as long as it includes git is okay, may be
nice to add it for 11 and 12 as well so it hits our default versions may be
nice as well, but not a requirement.
------------------------------
In pkgs/build-support/bintools-wrapper/default.nix
<#173498 (comment)>:
> + ##
+ ## Set the default machine type so that $prefix-ld.lld uses the COFF driver for --help
+ ##
+ ## Needed because autotools parses --help for linker features...
+ ##
+ + optionalString (isLld && stdenv.targetPlatform.isWindows) (let
+ mtype =
+ /**/ if targetPlatform.isx86_32 then "i386pe"
+ else if targetPlatform.isx86_64 then "i386pep"
+ else if targetPlatform.isAarch32 then "thumb2pe"
+ else if targetPlatform.isAarch64 then "arm64pe"
+ else throw "unsupported target arch for lld";
+ in ''
+ export mtype=${mtype}
+ substituteAll ${./add-lld-ldflags-before.sh} add-local-ldflags-before.sh
+ cat add-local-ldflags-before.sh >> $out/nix-support/add-local-ldflags-before.sh
Can you change the other writes to
$out/nix-support/add-local-ldflags-before.sh to append as well? I'm a bit
concerned that somebody starts overwriting additions accidentally by
reordering code in here.
------------------------------
In pkgs/development/compilers/llvm/13/bintools/default.nix
<#173498 (comment)>:
>
let
prefix =
if stdenv.hostPlatform != stdenv.targetPlatform
then "${stdenv.targetPlatform.config}-"
else "";
-in runCommand "llvm-binutils-${version}" { preferLocalBuild = true; } ''
+in runCommand "llvm-binutils-${version}" {
+ preferLocalBuild = true;
+ passthru = {
+ isLld = true;
+ targetPrefix = prefix;
Why don't the other LLVM versions have a targetPrefix passthru now?
------------------------------
In pkgs/development/compilers/llvm/13/bintools/default.nix
<#173498 (comment)>:
> @@ -26,4 +32,7 @@ in runCommand "llvm-binutils-${version}" { preferLocalBuild = true; } ''
ln -s ${llvm}/bin/llvm-strip $out/bin/${prefix}strip
ln -s ${lld}/bin/lld $out/bin/${prefix}ld
-''
+'' + lib.optionalString stdenv.targetPlatform.isWindows ''
+ ln -s ${llvm}/bin/llvm-windres $out/bin/${prefix}windres
+ ln -s ${llvm}/bin/llvm-dlltool $out/bin/${prefix}dlltool
+'')
Is this relevant to other LLVM versions as well? Note that unless you add
any changes to llvmPackages 14 and llvmPackages_git, it'll likely get lost
in the future.
------------------------------
In pkgs/development/tools/misc/libtool/libtool2.nix
<#173498 (comment)>:
> @@ -23,6 +23,20 @@ stdenv.mkDerivation rec {
# https://lists.gnu.org/archive/html/autotools-announce/2022-03/msg00000.html
FILECMD = "${file}/bin/file";
+ patches = [
+ # Patches from msys2 fixing various bugs with useClang platforms
+ # targeting Windows. Especially https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27866
+ ./0002-cygwin-mingw-Create-UAC-manifest-files.mingw.patch
+ ./0005-Fix-seems-to-be-moved.patch
+ ./0006-Fix-strict-ansi-vs-posix.patch
+ ./0007-fix-cr-for-awk-in-configure.all.patch
+ ./0010-libtool-2.4.2-include-process-h.patch
+ ./0011-Pick-up-clang_rt-static-archives-compiler-internal-l.patch
+ ./0012-Prefer-response-files-over-linker-scripts-for-mingw-.patch
+ ./0013-Allow-statically-linking-compiler-support-libraries-.patch
+ ./0014-Support-llvm-objdump-f-output.patch
+ ];
Can't say anything w.r.t. these patches. Maybe we can pester @aakropotkin
<https://github.com/aakropotkin> about this?
------------------------------
In pkgs/development/libraries/libffi/default.nix
<#173498 (comment)>:
> @@ -45,6 +47,8 @@ stdenv.mkDerivation rec {
checkInputs = [ dejagnu ];
+ nativeBuildInputs = lib.optional stdenv.hostPlatform.isWindows autoreconfHook;
Can you add a comment why this is needed? I.e. is this a structural
problem or will this fix itself in a future libffi release?
------------------------------
In pkgs/development/libraries/gmp/6.x.nix
<#173498 (comment)>:
> @@ -29,7 +29,7 @@ let self = stdenv.mkDerivation rec {
passthru.static = self.out;
depsBuildBuild = [ buildPackages.stdenv.cc ];
- nativeBuildInputs = [ m4 ];
+ nativeBuildInputs = [ m4 ] ++ lib.optional stdenv.hostPlatform.isWindows autoreconfHook;
Can you add a comment why this is needed? I.e. is this a structural
problem or will this fix itself in a future gmp release?
------------------------------
In pkgs/development/compilers/llvm/13/libcxx/default.nix
<#173498 (comment)>:
> @@ -41,7 +41,13 @@ stdenv.mkDerivation rec {
"-DLIBCXX_ENABLE_THREADS=OFF"
"-DLIBCXX_ENABLE_FILESYSTEM=OFF"
"-DLIBCXX_ENABLE_EXCEPTIONS=OFF"
- ] ++ lib.optional (!enableShared) "-DLIBCXX_ENABLE_SHARED=OFF";
+ ]
+ ++ lib.optional (!enableShared) "-DLIBCXX_ENABLE_SHARED=OFF"
+ ++ lib.optionals (!headersOnly && libcxxabi.semi-static) [
+ "-DLIBCXX_ENABLE_STATIC_ABI_LIBRARY=TRUE"
+ "-DLIBCXX_CXX_ABI_LIBRARY_PATH=${libcxxabi}/lib"
+ ] ++ lib.optional (!headersOnly && libcxxabi.useLLVMUnwinder)
+ "-DLIBCXXABI_USE_LLVM_UNWINDER=ON";
Still confused about this one…
------------------------------
In pkgs/development/compilers/llvm/13/libcxxabi/default.nix
<#173498 (comment)>:
>
-stdenv.mkDerivation rec {
+ enableShared' = enableShared && !semi-static;
Maybe onlyEnableShared is clearer?
—
Reply to this email directly, view it on GitHub
<#173498 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA3ORYRAH7LEHFIJJ5HLD3DVMXSKHANCNFSM5WIUDRZQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Here's the thread with the patches upstream that @aakropotkin mentions https://debbugs.gnu.org/cgi/bugreport.cgi?bug=27866 |
|
Well, we definitely can't apply libtool patches that have been rejected upstream and cause incorrect behavior in some cases. If they are indeed required for mingw64-llvm cross, we should make them conditional to that platform. Unfortunately this means someone needs to keep an eye on cross-trunk whenever libtool is updated. |
|
This is likely the safest move.
I do think that these patches are useful to add as a setup hook or
alternate build for libtool that can be applied on a per-project basis.
The email thread has more details but the largest issue I saw was that
really this is a patch that fundamentally alters parts of libtool,
conflicting with the existing UI, for all platforms and tools in order to
resolve an issue in a specific Windows build env that is able to use two
compilers and two link editors.
If you fall into this specific category of user, and you understand the way
that the UI has been altered, then I think it's a very useful set of
patches; but as written they do not limit the scope of changes to the
relevant environment, they are not optionally enabled, and they do not
update the relevant documentation about the UI differences.
…On Tue, May 31, 2022, 1:31 PM sternenseemann ***@***.***> wrote:
Well, we definitely can't apply libtool patches that have been rejected
upstream and cause incorrect behavior in some cases. If they are indeed
required for mingw64-llvm cross, we should make them conditional to that
platform. Unfortunately this means someone needs to keep an eye on
cross-trunk whenever libtool is updated.
—
Reply to this email directly, view it on GitHub
<#173498 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA3ORYTGOFUFP2F2S6PQJQTVMZLJDANCNFSM5WIUDRZQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Having interacted with some of the LLVM people there and you all, I would be happy to convene some meeting and hash out how things ought to work, if this a case of everyone hacking around each other. |
|
I'm up for it. A call or something would be good rather than an email chain
to avoid miscommunications and speed things along.
I'll be the first to admit that I really don't like how `libtool` handles
the flagspecs currently, and would really benefit from some help on
resolving it.
The biggest issue is honestly that I'm one person, and `libtool` patches
honestly just gets tested on my collection of VMs. I have Hydra building
which was a huge help, but no VMs there. Having more environments to run on
would be a life saver.
I have an idea of how I'd like libtool to handle this issue, but getting
there is daunting with the infrastructure I have set up currently.
If anyone could help make new tests as Hydra flake jobs that would even be
an enormous help. The existing test suite concerns me quite a bit, tons of
"remove this in 2009" with no further explanation floating around the
comments, and after driving it for the last six months I'm not convinced
that it's trustworthy; a lot of them seem to operate under the assumption
of FHS which is not ideal for today's containerized approaches. Having
Hydra jobs that just use libtool as a dependency to build something would
be significantly more useful.
…On Tue, May 31, 2022, 10:02 PM John Ericson ***@***.***> wrote:
Having interacted with some of the LLVM people there and you all, I would
be happy to convene some meeting and hash out how things *ought* to work,
if this a case of everyone hacking around each other.
—
Reply to this email directly, view it on GitHub
<#173498 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA3ORYRVDATWV6ORRSGRDRTVM3HF3ANCNFSM5WIUDRZQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Draft until I can conditionalize libtool patches and respond to other comments. |
92 tasks
stale
bot
added
the
2.status: stale
|
Did this get done in the meantime or something? |
|
No, too much complexity to finish quickly and the work project got cancelled. |
rrbutani
added
the
6.topic: llvm/clang
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Waiting on next staging cycle to move forward.
This reverts commit c911240.