-
Notifications
You must be signed in to change notification settings - Fork 84
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 'cc --print-sysroot' call #258
Fix 'cc --print-sysroot' call #258
Conversation
Cflags should probably be added upstream. |
I was doing some work on MbedTLS upstream today and ran into what I think was |
I am not sure what you mean exactly. By "upstream" do you mean the upstream MbedTLS repository? Then I would agree that the issue should be fixed there because the function which contains the unused variable does not even exist anymore in the current state of the repository. But here, we have included the "vendor" directory which contains an older release. As soon as we change something there, we are effectively shipping something different. So I do not know if that is acceptable. If it is, I would try to refine the preconditions under which the |
Upstream means https://github.com/Mbed-TLS/mbedtls/. I'd rather pick up whatever change they have, if possible, instead of some new change you came up with. |
That's what we should ultimately do, for sure. I've already considered this before:
But we are shipping version 2.28 from December 2021 and my change is fixing trouble caused by a compiler feature introduced/released in October 2021, around the same time. So maybe they just haven't tested it with clang 13 back then. Now, our builds fail because we rely on an old mbedtls version, a fix there would be nice, I guess. |
So what are your plans here? Do we want to fix the broken build or do we just accept that the older mbedtls version shipped in the |
@DrTobe According to https://cmake.org/cmake/help/latest/envvar/CFLAGS.html#envvar:CFLAGS , it seems you could use environment variable Note this |
This does not work because in the resulting I have seen that in 60d5c27 and 34448f9, the Alternatively, I thought about patching the vendor directory but this will make the build process even more complicated and harder to understand. Regarding the fact that the In the meantime, I have also checked the upstream mbedtls-2.28.x releases. The official issue to this problem was Mbed-TLS/mbedtls#7166 which was fixed in PR Mbed-TLS/mbedtls#7167 with commit Mbed-TLS/mbedtls@d784833. This fix first appears in mbedtls-2.28.3 so if upgrading the vendored mbedtls version is not too hard, we could (and should for security fixes?) aim to upgrade to 2.28.3 until the upgrade to mbedtls-3.x.y is done? |
I see, will look into if it's fine to upgrade |
@DrTobe as mentioned, I'd like to understand how upstream fixed this (assuming they do support clang 14) before accepting any change here. |
Initially, I had overlooked this fix because I had checked the current |
Should I remove the unused variable fix so that we can merge the fix for the |
If updating to upstream 2.28.3 fixes this, let's just do that instead of this PR. |
Ok |
4f835df
to
9664203
Compare
Done. I guess this should be good to merge now. |
bors r+ |
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page.
|
This PR contains two changes:
cc --print-sysroot
is not properly checked before it is evaluated and added as a compiler flag. This one should be easy to accept.vendor
directory which should probably stay untouched? I just did not know how to append a compiler flag to the CMake process without doing so.Without the new compiler flag
-Wno-unused-but-set-variable
, Apple clang 14 fails with the following error message and this conclusion seems to be correct:So to fix builds for compilers which properly detect this, we need to somehow change the compiler flags for the C code. Setting it with
cmk.cflag(...)
inmbedtls-sys/build/cmake.rs
did not work. I have thought about settingCMAKE_C_FLAGS
before calling CMake but the new flag must appear behind-Wall
which is set by theCMakeLists.txt
so I guess this will not work either. So how would you like to do this? Any other ideas?Edit: The second commit has been removed (see discussion)