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 hardcoded FEATURE_DISTRO_AGNOSTIC_SSL #1755

Merged
merged 1 commit into from
Sep 24, 2020

Conversation

omajid
Copy link
Member

@omajid omajid commented Sep 16, 2020

This is a fix for dotnet/runtime#41768

@omajid
Copy link
Member Author

omajid commented Sep 17, 2020

cc @tmds

@dagood
Copy link
Member

dagood commented Sep 17, 2020

macOS error is a homebrew issue, fixed here: Homebrew/homebrew-cask-versions#9633. (I hit retry in case you wanted to run this on macOS too to see if it regresses there.)

@omajid
Copy link
Member Author

omajid commented Sep 17, 2020

I just realized that this doesn't handle the case where FEATURE_DISTRO_AGNOSTIC_SSL is defined... because it's never defined in the installer configuration, only in libraries 😢

@omajid omajid force-pushed the optional-distro-agnostic-ssl branch from efea6f4 to cae295d Compare September 17, 2020 22:27
@omajid
Copy link
Member Author

omajid commented Sep 18, 2020

dotnet/runtime PR: dotnet/runtime#42415

@omajid
Copy link
Member Author

omajid commented Sep 21, 2020

The dotnet/runtime PR was merged, into master EDIT: I misread the state. It's still in review. But it's still targeted for master.

That means this wont be a part of 5.0 GA, right? What's the best thing to do from a source-build point of view now? Should we get this PR merged or do something else?

@dagood
Copy link
Member

dagood commented Sep 21, 2020

That means this wont be a part of 5.0 GA, right?

Yeah, my understanding is master is solidly 6.0 at this point, no automatic backporting.

What's the best thing to do from a source-build point of view now? Should we get this PR merged or do something else?

I think that we should get this patch in to make sure it keeps building correctly in dotnet/source-build up to 5.0 GA no matter what. Then we can remove it naturally once it gets ported to 5.0 (whether that's before or after GA).

I think there's just the one nit from the dotnet/runtime PR to bring back here, the message. I think we should put a link to the dotnet/runtime PR in the patch description to make it easy to navigate later. (E.g. if it's auto-applied, show up nicely in the commit message.) Then I think it's good to go.

@crummel @dseefeld

@omajid omajid force-pushed the optional-distro-agnostic-ssl branch from cae295d to 7aca930 Compare September 23, 2020 16:44
Backport dotnet/runtime#42415 and carry it as a
patch for now.
@omajid omajid force-pushed the optional-distro-agnostic-ssl branch from 7aca930 to 547649b Compare September 23, 2020 17:07
set(START_WHOLE_ARCHIVE -Wl,--whole-archive)
@@ -212,3 +217,10 @@ target_link_libraries(singlefilehost
${NATIVE_LIBS}
${END_WHOLE_ARCHIVE}
Copy link
Member Author

@omajid omajid Sep 24, 2020

Choose a reason for hiding this comment

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

I had to manually massage these few lines for the patch to apply. These were changed between Preview 8 and master.

@omajid omajid changed the title WIP: Fix hardcoded FEATURE_DISTRO_AGNOSTIC_SSL Fix hardcoded FEATURE_DISTRO_AGNOSTIC_SSL Sep 24, 2020
@dagood dagood merged commit 07d4bb3 into dotnet:master Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants