Skip to content
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.sh --portableBuild false #50097

Merged
merged 1 commit into from
Apr 19, 2021

Conversation

omajid
Copy link
Member

@omajid omajid commented Mar 23, 2021

Non-portable builds are used by source-build and also useful for testing/debugging things, like trying out OpenSSL 3.0:
#46526 (comment)

Fixes: #43219

Should I add a CI run that verifies ./build.sh --portableBuild false works?

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@omajid omajid force-pushed the fix-non-portable-build branch from 9227ad5 to 97c21fa Compare March 23, 2021 14:06
@omajid omajid mentioned this pull request Mar 23, 2021
12 tasks
@ghost
Copy link

ghost commented Mar 24, 2021

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

Non-portable builds are used by source-build and also use for testing/debugging things, like trying out OpenSSL 3.0:
#46526 (comment)

Fixes: #43219

Should I add a CI run that verifies ./build.sh --portableBuild false works?

Author: omajid
Assignees: -
Labels:

area-Infrastructure

Milestone: -

@danmoseley
Copy link
Member

Should I add a CI run that verifies ./build.sh --portableBuild false works?

cc @ViktorHofer for that question.

@omajid
Copy link
Member Author

omajid commented Mar 24, 2021

FWIW, having a CI leg for this would have caught #42415 sooner

@omajid
Copy link
Member Author

omajid commented Mar 24, 2021

I can't tell if the linux-musl-arm error is related to my changes or not. I would have understood a failure building/running ilasm/ildasm, but this is something else?

@omajid omajid force-pushed the fix-non-portable-build branch from 97c21fa to f78f38b Compare March 25, 2021 13:36
@omajid omajid force-pushed the fix-non-portable-build branch from f78f38b to f87df0c Compare April 7, 2021 20:13
@omajid omajid changed the title WIP: Fix ./build.sh portableBuild false Fix ./build.sh portableBuild false Apr 8, 2021
@omajid omajid changed the title Fix ./build.sh portableBuild false Fix ./build.sh --portableBuild false Apr 8, 2021
@omajid omajid closed this Apr 13, 2021
@omajid omajid reopened this Apr 13, 2021
@omajid omajid force-pushed the fix-non-portable-build branch from f87df0c to 0716d25 Compare April 13, 2021 19:01
@omajid
Copy link
Member Author

omajid commented Apr 14, 2021

Can someone please review this? The test failures look unrelated to my changes.

@ViktorHofer
Copy link
Member

Should I add a CI run that verifies ./build.sh --portableBuild false works?

I think we should have a non portable build in CI to protect that scenario. Unsure if per PR or as a rolling build.

@ViktorHofer ViktorHofer requested a review from a team April 14, 2021 15:34
@omajid
Copy link
Member Author

omajid commented Apr 14, 2021

#50811 (comment):

once #50097 gets in, we can add the --portableBuild false flag to this leg.

@ViktorHofer
Copy link
Member

So we probably wanna do this as part of this PR now as the other change is already merged?

@omajid omajid force-pushed the fix-non-portable-build branch from 0716d25 to f015252 Compare April 14, 2021 16:19
@ViktorHofer
Copy link
Member

Thanks a lot @omajid

@omajid
Copy link
Member Author

omajid commented Apr 14, 2021

Hey, @bartonjs, this new CI run is failing on CentOS 7 (with OpenSSL 1.0 non-portable):

$ podman run -it mcr.microsoft.com/dotnet-buildtools/prereqs:centos-7-source-build-20210408124356-5d87b80 bash -c 'rpm -qa | grep openssl'
openssl-devel-1.0.2k-21.el7_9.x86_64
openssl-libs-1.0.2k-21.el7_9.x86_64

The errors look like this:

/__w/1/s/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_evp_pkey_rsa.c:88:26: error: implicit declaration of function 'EVP_PKEY_get0_RSA' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
          const RSA* rsa = EVP_PKEY_get0_RSA(pkey);
                           ^
  /__w/1/s/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_evp_pkey_rsa.c:88:26: note: did you mean 'EVP_PKEY_get1_RSA'?
  /usr/include/openssl/evp.h:968:16: note: 'EVP_PKEY_get1_RSA' declared here
  struct rsa_st *EVP_PKEY_get1_RSA(EVP_PKEY *pkey);
                 ^
  /__w/1/s/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_evp_pkey_rsa.c:88:26: error: this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
          const RSA* rsa = EVP_PKEY_get0_RSA(pkey);
                           ^
  /__w/1/s/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_evp_pkey_rsa.c:88:20: error: incompatible integer to pointer conversion initializing 'const RSA *' (aka 'const struct rsa_st *') with an expression of type 'int' [-Werror,-Wint-conversion]
          const RSA* rsa = EVP_PKEY_get0_RSA(pkey);
                     ^     ~~~~~~~~~~~~~~~~~~~~~~~
  /__w/1/s/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_evp_pkey_rsa.c:147:51: error: use of undeclared identifier 'RSA_PSS_SALTLEN_DIGEST'
              EVP_PKEY_CTX_set_rsa_pss_saltlen(ctx, RSA_PSS_SALTLEN_DIGEST) <= 0)
                                                    ^
  /__w/1/s/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_evp_pkey_rsa.c:163:26: error: implicit declaration of function 'EVP_PKEY_get0_RSA' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
          const RSA* rsa = EVP_PKEY_get0_RSA(pkey);
                           ^
  /__w/1/s/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_evp_pkey_rsa.c:163:20: error: incompatible integer to pointer conversion initializing 'const RSA *' (aka 'const struct rsa_st *') with an expression of type 'int' [-Werror,-Wint-conversion]
          const RSA* rsa = EVP_PKEY_get0_RSA(pkey);
                     ^     ~~~~~~~~~~~~~~~~~~~~~~~

Are these known issues?

@bartonjs
Copy link
Member

Are these known issues?

I was originally going to say "sort of, I saw and fixed this in #51155". But that appears to be untrue.

#define EVP_PKEY_get0_RSA local_EVP_PKEY_get0_RSA

belongs right above

#define EVP_PKEY_up_ref local_EVP_PKEY_up_ref

@bartonjs
Copy link
Member

Actually, since there's also a PSS_SALTLEN problem it requires slightly more surgery. (The "define if missing" needs to move up to before the "#if portable")

https://github.com/dotnet/runtime/pull/51155/files#diff-c041469cc460b750021244a4a434fedb53956c4aad395137c83ab37583b4e840R73-R76

https://github.com/dotnet/runtime/pull/51155/files#diff-c041469cc460b750021244a4a434fedb53956c4aad395137c83ab37583b4e840L221-L227

@omajid omajid force-pushed the fix-non-portable-build branch 2 times, most recently from 5e11bfc to c9e2a17 Compare April 14, 2021 21:31
@bartonjs
Copy link
Member

@omajid Looks like you didn't quite move it high enough. If you pull the newer main you should be able to get rid of the PSS_SALTLEN move (since I already did it and it's now a conflict), but adding the missing #define EVP_PKEY_get0_RSA local_EVP_PKEY_get0_RSA is still necessary.

Non-portable builds are used by source-build and also use for
testing/debugging things, like trying out OpenSSL 3.0:
dotnet#46526 (comment)

Fixes: dotnet#43219
@omajid omajid force-pushed the fix-non-portable-build branch from c9e2a17 to 4fa7e11 Compare April 15, 2021 14:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

./build.sh --portablebulid false fails
5 participants