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 bad configure tests #42756

Merged
merged 1 commit into from
Sep 29, 2020

Conversation

janvorli
Copy link
Member

There was an extra -c in the CMAKE_REQUIRED_FLAGS set for testing
HAVE_UNW_GET_ACCESSORS and HAVE_UNW_GET_SAVE_LOC that was breaking
build of coreclr under homebrew. The option was somehow making
these checks behave on ARM Linux, eveb though it is not clear to
me why, as it was just causing this option to be passed to the
compiler twice at different positions of the command line of
the cmake tests.
This change fixes it by using check_symbol_exists instead of
check_c_source_compiles, since just removing the duplicite -c
was resulting in the check failing on ARM / ARM64 Linux due
to a missing symbol from libunwind during linking.

There was an extra -c in the CMAKE_REQUIRED_FLAGS set for testing
HAVE_UNW_GET_ACCESSORS and HAVE_UNW_GET_SAVE_LOC that was breaking
build of coreclr under homebrew. The option was somehow making
these checks behave on ARM Linux, eveb though it is not clear to
me why, as it was just causing this option to be passed to the
compiler twice at different positions of the command line of
the cmake tests.
This change fixes it by using check_symbol_exists instead of
check_c_source_compiles, since just removing the duplicite -c
was resulting in the check failing on ARM / ARM64 Linux due
to a missing symbol from libunwind during linking.
@janvorli janvorli added this to the 5.0.0 milestone Sep 25, 2020
@janvorli janvorli requested a review from jkotas September 25, 2020 20:06
@janvorli janvorli self-assigned this Sep 25, 2020
@janvorli
Copy link
Member Author

cc: @omajid

Copy link
Member

@omajid omajid left a comment

Choose a reason for hiding this comment

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

A change that makes things more broadly compatible and removes more code than it adds? 🎉

@asbjornu
Copy link
Member

asbjornu commented Oct 1, 2020

Awesome! What's the ETA of this becoming available in source-build?

omajid added a commit to omajid/dotnet-source-build that referenced this pull request Oct 2, 2020
See: dotnet#1744

This is a backport of the fix that was merged into runtime for master:
dotnet/runtime#42756
@dagood
Copy link
Member

dagood commented Oct 2, 2020

Will this be backported for 5.0.0?

@omajid
Copy link
Member

omajid commented Oct 2, 2020

Awesome! What's the ETA of this becoming available in source-build?

If dotnet/source-build#1789 is merged, the next monthly release (mid-October? mid-November?) tag will include the fix.

@dagood
Copy link
Member

dagood commented Oct 2, 2020

(mid-October? mid-November?)

It's too late for the October tag unless there's a build reset (not expected), but if you're ok using a commit rather than a tag (I think you mentioned this is ok at some point?), you can start using it immediately after dotnet/source-build#1789 is merged.

@asbjornu
Copy link
Member

asbjornu commented Oct 2, 2020

Cool, I'll definitely update the Homebrew Formula pointing to the merge commit SHA once dotnet/source-build#1789 is merged.

@dagood
Copy link
Member

dagood commented Oct 5, 2020

/backport to release/5.0

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2020

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/289510941

@dagood
Copy link
Member

dagood commented Oct 5, 2020

I figure whether or not it'll make 5.0.0 itself, might as well kick it off. Saw people using that incantation in other PRs. 😛

dagood pushed a commit to dotnet/source-build that referenced this pull request Oct 5, 2020
See: #1744

This is a backport of the fix that was merged into runtime for master:
dotnet/runtime#42756
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
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.

5 participants