Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

[release/2.1] Fix build errors in some build configurations #35805

Merged
merged 2 commits into from
Mar 7, 2019

Conversation

omajid
Copy link
Member

@omajid omajid commented Mar 6, 2019

There's two issues that cause different build warnings, breaking the build under certain conditions:

Issue 1

This was a mistake in e4bcbd5 (#34443) made when backporting it to the release branches. All other uses of NEED_OPENSSL_1_0 are guarded by an #ifdef, not an #if. This one should be too.

The warning is seen when building corefx using source-build. I couldn't observe it directly:

source-build/src/corefx/src/Native/Unix/System.Security.Cryptography.Native/pal_asn1_print.cpp:34:5:
error: 'NEED_OPENSSL_1_0' is not defined, evaluates to 0 [-Werror,-Wundef]
  #if NEED_OPENSSL_1_0
  ^

Issue 2

When building in non-portable mode, some OpenSSL 1.1 function definitions that are marked as unused can be picked up by our build. When those functions are called, clang reports a warning and fails the build:

src/Native/Unix/System.Security.Cryptography.Native/openssl.cpp:432:12:
error: 'sk_ASN1_OBJECT_num' was marked unused but was used [-Werror,-Wused-but-marked-unused]
      return sk_ASN1_OBJECT_num(eku);
             ^

This 'unused' attribute was recently added to sk_* methods in OpenSSL 1.1: openssl/openssl#8246

cc @bartonjs

This was a mistake in e4bcbd5 (dotnet#34443) made when backporting it to
the release branches. All other uses of NEED_OPENSSL_1_0 are guarded by
an #ifdef, not an #if. This one should be too.

The warning is seen when building corefx using source-build. I couldn't observe
it directly:

    source-build/src/corefx/src/Native/Unix/System.Security.Cryptography.Native/pal_asn1_print.cpp:34:5:
    error: 'NEED_OPENSSL_1_0' is not defined, evaluates to 0 [-Werror,-Wundef]
      #if NEED_OPENSSL_1_0
      ^
@bartonjs
Copy link
Member

bartonjs commented Mar 6, 2019

Do both ./build-native.sh and ./build-native.sh -portable from src/Native/Unix/ work now?

@omajid
Copy link
Member Author

omajid commented Mar 6, 2019

No, ./build-native.sh doesn't work. Looking into fixing other warnings now.

@omajid
Copy link
Member Author

omajid commented Mar 6, 2019

When OpenSSL 1.0 development headers and libraries are installed: both ./build-native.sh and ./build-native.sh -portable work.

When OpenSSL 1.1 development headers and libraries installed: ./build-native.sh fails, but that error exists in master too: https://github.com/dotnet/corefx/issues/35807

@davidsh davidsh added this to the 3.0 milestone Mar 6, 2019
@davidsh davidsh added area-System.Security os-linux Linux OS (any supported distro) labels Mar 6, 2019
@bartonjs bartonjs modified the milestones: 3.0, 2.1.x Mar 6, 2019
…hy.Native

When building in non-portable mode, some OpenSSL 1.1 function defnitions
that are marked as unused can be picked up by our build. When those
functions are called, clang reports a warning and fails the build:

    src/Native/Unix/System.Security.Cryptography.Native/openssl.cpp:432:12:
    error: 'sk_ASN1_OBJECT_num' was marked unused but was used [-Werror,-Wused-but-marked-unused]
          return sk_ASN1_OBJECT_num(eku);
                 ^

This 'unused' attribute was recently added to sk_* methods in OpenSSL
1.1: openssl/openssl#8246
@omajid omajid changed the title Change #if NEED_OPENSSL_1_0 to #ifdef NEED_OPENSSL_1_0 Fix build warnings in some build configurations Mar 6, 2019
@bartonjs
Copy link
Member

bartonjs commented Mar 6, 2019

Annoying re-verifying: Do both the portable and non-portable build-native.shs work on your machine now? :)

@omajid
Copy link
Member Author

omajid commented Mar 6, 2019

Yes, both ./build-native.sh and ./build-native.sh -portable work with OpenSSL 1.1 headers installed. I didn't check OpenSSL 1.0 again.

Copy link
Member

@bartonjs bartonjs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI @danmosemsft

@bartonjs bartonjs changed the title Fix build warnings in some build configurations [release/2.1] Fix build errors in some build configurations Mar 6, 2019
@omajid
Copy link
Member Author

omajid commented Mar 6, 2019

Tested with OpenSSL 1.0 installed: both ./build-native.sh and ./build-native.sh -portable work too.

@vivmishra vivmishra added the Servicing-approved Approved for servicing release label Mar 7, 2019
@vivmishra vivmishra modified the milestones: 2.1.x, 2.1.10 Mar 7, 2019
@vivmishra
Copy link

Approved for 2.1.10 and 2.2.4.

@bartonjs bartonjs merged commit e713bb5 into dotnet:release/2.1 Mar 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security os-linux Linux OS (any supported distro) Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants