Fix 'cc --print-sysroot' call

[DrTobe]

This PR contains two changes:

1. The first commit fixes a bug that the result of cc --print-sysroot is not properly checked before it is evaluated and added as a compiler flag. This one should be easy to accept.
2. The second commit is harder to accept because it changes code in the 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:

mbedtls-sys/vendor/library/bignum.c:1395:29: error: variable 't' set but not used [-Werror,-Wunused-but-set-variable]
      mbedtls_mpi_uint c = 0, t = 0;
                              ^

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(...) in mbedtls-sys/build/cmake.rs did not work. I have thought about setting CMAKE_C_FLAGS before calling CMake but the new flag must appear behind -Wall which is set by the CMakeLists.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)

[jethrogb]

Cflags should probably be added upstream. But you could look at https://github.com/fortanix/rust-mbedtls/blob/master/mbedtls-sys/build/build.rs#L99 You already did that

[jethrogb]

I was doing some work on MbedTLS upstream today and ran into what I think was -Wunused-but-set-variable. I think this is fixed in the latest version of MbedTLS upstream. We generally accept upstream patches into the vendor directory, so that may work out here.

[DrTobe]

I was doing some work on MbedTLS upstream today and ran into what I think was -Wunused-but-set-variable. I think this is fixed in the latest version of MbedTLS upstream. We generally accept upstream patches into the vendor directory, so that may work out here.

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 -Wno-unused-but-set-variable flag is appended (it seems to require a minimum clang version which I do not know yet, I'll have to figure this out). All in all, this should just be a temporary solution until we upgrade the mbedtls release in the vendor directory.

[jethrogb]

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.

[DrTobe]

That's what we should ultimately do, for sure. I've already considered this before:

All in all, this should just be a temporary solution until we upgrade the mbedtls release in the vendor directory.

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.

[DrTobe]

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 vendor directory just does not build with newer clang versions, effectively causing rust-mbedtls to not build with newer clang versions?

[Taowyoo]

@DrTobe According to https://cmake.org/cmake/help/latest/envvar/CFLAGS.html#envvar:CFLAGS , it seems you could use environment variable CFLAGS to init the CMAKE_C_FLAGS with -Wno-unused-but-set-variable, the C mbedtls 's CMakeList.txt also only do append on CMAKE_C_FLAGS

Note this CFLAGS is not the cflag in build.rs, it's the environment variable

[DrTobe]

This does not work because in the resulting cc invocation, the -Wno-unused-but-set-variable flag appears before the -Wall flag. The -Wall (re)activates the unused-but-set-variable warning, i.e. the -Wno-unused-but-set-variable is overridden and effectively useless. To really ignore this warning/error, we need to append the -Wno-unused-but-set-variable flag after -Wall. Until now, I do not know how to do so without touching the vendor directory.

I have seen that in 60d5c27 and 34448f9, the vendor directory has been touched before. So the version we ship is already not 100% identical to the official release. So maybe, we could also merge this change if we do not find any other solution?

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 vendor directory has already diverged from the official release, I would rather not do so.

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?

[Taowyoo]

I see, will look into if it's fine to upgrade mbedtls-sys to 2.28.3

[jethrogb]

@DrTobe as mentioned, I'd like to understand how upstream fixed this (assuming they do support clang 14) before accepting any change here.

[DrTobe]

@jethrogb

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.

Initially, I had overlooked this fix because I had checked the current development branch which apparently has been reworked quite a bit so that the function where the unused variable is located does not exist anymore. The upstream fix is a (void) t; statement.

[DrTobe]

I see, will look into if it's fine to upgrade mbedtls-sys to 2.28.3

Should I remove the unused variable fix so that we can merge the fix for the cc --print-sysroot call?

[jethrogb]

If updating to upstream 2.28.3 fixes this, let's just do that instead of this PR.

[jethrogb]

Should I remove the unused variable fix so that we can merge the fix for the cc --print-sysroot call?

Ok

[DrTobe]

Should I remove the unused variable fix so that we can merge the fix for the cc --print-sysroot call?

Done. I guess this should be good to merge now.

[jethrogb]

bors r+

[bors]

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.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• continuous-integration/travis-ci/push