-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
nushell: update from 0.18.1 to 0.20.0 #5773
Conversation
@@ -28,6 +28,9 @@ termux_setup_rust() { | |||
ENV_NAME=${ENV_NAME//-/_} | |||
export $ENV_NAME=$CC | |||
export TARGET_CFLAGS="$CFLAGS $CPPFLAGS" | |||
# This was getting applied for the host build of Rust macros or whatever, so | |||
# unset it. | |||
unset CFLAGS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@its-pointless, let me know if this is okay to remove for all crates, as the target flag is set just above and this one was getting picked up by C crates that are built for the host, for macros or whatever, and breaking the build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xeffyr or @Grimler91, any opinion on removing this flag for all crates that use C code? I believe it was wrong to keep this, but I don't use Rust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, without unsetting you were getting build errors during some host compilation for nushell due to unknown flags?
Looks like a reasonable change, but might be a good idea to test build a couple of rust packages after the flag has been unset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but not unknown, the CI error showed that the problem was these cross-compilation flags wrongly being applied to the host build (see the compiler command used and resulting error).
I don't think testing other Rust packages will show anything, as this would have broken them already, just like the previous version of nushell built fine. The problem only came up when nushell started depending on OpenSSL in this version, and compiling that C crate dependency for the host finally surfaced this bug.
I will try building a few Rust packages anyway though, to make sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried compiling the librsvg, ripgrep, and starship packages with this patch applied, no problem. I will look into a fix for the x86_64 build and then merge.
It looks like the linker can't find the libc.so in the sysroot at link time. its also using /usr/bin/ld hmmm or it could be its trying to link against libssl in termux directory for host build instead of usual location. |
7636585
to
7710c47
Compare
The latter, the host build is being passed the Termux prefix to link against for some reason and fails only for x86_64, since the target arch is the same as the host. It's not because of |
i got it working https://github.com/its-pointless/termux-packages/blob/nushell/packages/nushell/build.sh |
Heh, that's a lot of work to avoid what is probably a cargo problem, passing a cross-compilation sysroot to the host build. |
23d5ecc
to
5f9bf8f
Compare
…ing issues, and unset CFLAGS for all crates, as it was getting applied to host builds
Alright, might as well get this in, can come back to x86_64 later. |
Work this out on the CI, here we go.