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

Consolidate objcopy detection #33342

Merged
merged 1 commit into from
Mar 10, 2020
Merged

Consolidate objcopy detection #33342

merged 1 commit into from
Mar 10, 2020

Conversation

am11
Copy link
Member

@am11 am11 commented Mar 8, 2020

strip_destination_file file is defined in :/eng/native/functions.cmake, which is already included by top-level cmake scripts in coreclr, installer and libraries. This PR de-duplicates that logic and simplifies condition blocks in eng/native/configuretool.cmake (where unified tool detection takes place).

@am11
Copy link
Member Author

am11 commented Mar 8, 2020

@janvorli, @jkotas, can you please take a look?
Failure is about SDK not found

curl: (22) The requested URL returned error: 404 Not Found
dotnet-install: Cannot download: https://dotnetcli.azureedge.net/dotnet/Sdk/5.0.100-preview.2.20155.14/dotnet-dev-ubuntu.16.04-x64.5.0.100-preview.2.20155.14.tar.gz
dotnet_install: Error: Could not find/download: `.NET Core SDK` with version = 5.0.100-preview.2.20155.14
dotnet_install: Error: Refer to: https://aka.ms/dotnet-os-lifecycle for information on .NET Core support
##[error](NETCORE_ENGINEERING_TELEMETRY=InitializeToolset) Failed to install dotnet SDK from public location (exit code '1').
/root/runtime/eng/common/tools.sh: line 418: /root/runtime/.dotnet/dotnet: No such file or directory
##[error](NETCORE_ENGINEERING_TELEMETRY=Build) Build failed (exit code '1').

@jkotas jkotas requested review from jkoritzinsky and janvorli March 8, 2020 14:55
@@ -1,4 +1,5 @@
include(CheckPIESupported)
include(${CMAKE_CURRENT_LIST_DIR}/functions.cmake)
Copy link
Member Author

Choose a reason for hiding this comment

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

If we try to invoke libraries build on unsupported platform, we get error from line 27 in this file: clr_unknown_arch macro undefined, instead of Only AMD64, ARM64, ARM and I386 are supported message from that function (defined in functions.cmake). Therefore, this inclusion here makes sense (as opposed to including functions.cmake before configureplatform.cmake at all callsites).

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you! I didn't even know that we have the strip_symbols at so many places.

@am11
Copy link
Member Author

am11 commented Mar 9, 2020

Thanks. There is still install_symbols and install_library_and_symbols functions in installer and libraries respectively, which could be moved to functions.cmake. The libraries has hard-coded destination PDB which makes it a bit different, so I did not touch it as part of this PR.

@jkotas jkotas merged commit 253a0b4 into dotnet:master Mar 10, 2020
@am11 am11 deleted the feature/consolidate-objcopy branch March 10, 2020 02:00
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 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.

4 participants