-
Notifications
You must be signed in to change notification settings - Fork 4.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
Fix build on Clear Linux and Android device #34211
Conversation
The Android change looks good but I'm not so sure about the CFLAGS/CXXFLAGS one. The reason why Clear Linux and other distros set these CFLAGS is exactly because they want them to be used by projects. It is standard behavior of autotools-based projects to be able to inject your own CFLAGS via env variables. |
Thanks. This project has complex configuration graph and desired flags are anyway materialized in configuration files, so some/most of the C{XX}Flags from environment are overwritten (unless cmake cache variable is used?) On Clear Linux, main problem is that If support of C{XX}Flags from the environment is desired (which I agree is the conventional approach), we can:
or do nothing. 🙂 |
If unsetting |
New changes pushed:
|
Seems like a disconnection issue on CI:
@jkotas, could you please run the failing legs? |
src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_dsa.c
Outdated
Show resolved
Hide resolved
@akoeplinger, updated the top comment. Could you please give it another pass? :) |
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.
LGTM, thank you!
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.
LGTM, thank you!
Although very close to the area which this PR is touching, but
|
OSX machine recycled during the run (#34472) |
@akoeplinger Are you ok with this change after the discussion? |
@jkotas, can this be merged? |
I am waiting for @akoeplinger to clear his changes requested feedback. The merging is blocked because of it. It is possible to merge anyway using admin privileges, but I prefer to use my admin privileges to handle emergencies only. |
Linux musl arm64 release looks similar to dotnet/arcade#4190. Could be due to #34044 ( cc @mmitche: https://dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_apis/build/builds/592074/logs/1016
|
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.
Looks good to me apart from one question. Sorry for the delay.
@@ -21,7 +21,6 @@ export PYTHON | |||
usage_list+=("-nopgooptimize: do not use profile guided optimizations.") | |||
usage_list+=("-pgoinstrument: generate instrumented code for profile guided optimization enabled binaries.") | |||
usage_list+=("-skipcrossarchnative: Skip building cross-architecture native binaries.") | |||
usage_list+=("-skiprestoreoptdata: skip restoring optimization data.") |
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.
@am11 this seems like an unintentional change?
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.
@akoeplinger, I called it out here #34211 (comment). This option in help menu was removed from code, the case-esac block which handles options, and superseded by -nopgooptimize
. For example, to quickly build coreclr native components with PAL tests on an unsupported Unix (e.g. SunOS, QNX), where dontet(1)
/msbuild is currently unavailable, we can use:
./src/coreclr/build-runtime.sh -skipgenerateversion -nopgooptimize \
-cmakeargs -DCLR_CMAKE_BUILD_TESTS=1
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.
Ok. It's still in src/coreclr/build-runtime.cmd that's why I was confused :)
Remaining failures are unrelated to PR changes.
Extraction failed for file: /datadisks/disk1/workspace/_work/1/s/__download__/libraries_test_assets_Linux_x64_Debug/libraries_test_assets_Linux_x64_Debug.tar.gz
code: 2
stdout: helix/
helix/tests/
helix/tests/Linux.AnyCPU.Debug/
helix/tests/Linux.AnyCPU.Debug/System.ComponentModel.Composition.Tests.zip
...
helix/tests/Linux.AnyCPU.Debug/System.IO.Compression.Brotli.Tests.zip
stderr:
gzip: stdin: unexpected end of file
/bin/tar: Unexpected EOF in archive
/bin/tar: Unexpected EOF in archive
/bin/tar: Error is not recoverable: exiting now
error: undefined; |
Consists of two small commits:
Fix native component build for Clear Linux
_FORTIFY_SOURCE=2
which conflictst with CoreCLR and Libraries compilation and break the build. this delta clears_FORTIFY_SOURCE=2
fromCMAKE_CXX_FLAGS
andCMAKE_C_FLAGS
if set by the environment (i.e. very early in the process).Only skip call to check_pie_supported on Android
glob(3)
, see the comment:runtime/eng/common/cross/build-android-rootfs.sh
Line 23 in f79afdb
-no-pie
flag, thus the call tocheck_pie_supported
fails. However, it sets the PIC flag for us (not sure exactly where?). With level 28, it allows -no-pie, and also sets PIC flag. Hence, the current master is happy for both use-cases.android.toolchain.cmake
into use).check_pie_supported
, and set PIC unconditionally (as Android requires PIC and setting it twice in certain scenario has no side-effect).cc @jkotas, @akoeplinger, @janvorli