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

Always use live host #71725

Merged
merged 21 commits into from
Jul 18, 2022
Merged

Always use live host #71725

merged 21 commits into from
Jul 18, 2022

Conversation

agocke
Copy link
Member

@agocke agocke commented Jul 6, 2022

Eliminates package dependency on the host during build and test.

Eliminates package dependency on the host during build and test.
@ghost
Copy link

ghost commented Jul 6, 2022

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

Issue Details

Eliminates package dependency on the host during build and test.

Author: agocke
Assignees: agocke
Labels:

area-Infrastructure-libraries

Milestone: -

@ViktorHofer ViktorHofer requested review from akoeplinger and a team July 6, 2022 20:05
@steveisok steveisok requested a review from directhex July 6, 2022 20:14
eng/liveBuilds.targets Outdated Show resolved Hide resolved
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
@am11
Copy link
Member

am11 commented Jul 15, 2022

--- a/eng/native/configuretools.cmake
+++ b/eng/native/configuretools.cmake
@@ -29,6 +29,7 @@ if(NOT WIN32 AND NOT CLR_CMAKE_TARGET_BROWSER)
     endif()
 
     find_program(EXEC_LOCATION_${exec}
+      NO_CACHE
       NAMES
       "${TOOLSET_PREFIX}${exec}${CLR_CMAKE_COMPILER_FILE_NAME_VERSION}"
       "${TOOLSET_PREFIX}${exec}")

fixes the build. "Building with older llvm" was fixed in #71742 but I forgot about this OOTB caching mechanism of cmake. :)

akoeplinger pushed a commit that referenced this pull request Jul 15, 2022
Unblocks #71725.
Followup: #71742

Prior to #71742, we were calling `locate_toolchain_exec` once per tool. Calling it twice with different prefix gives us the old result, which is something we don't want (we only call this local function a few times in this file, and each time for intention to "lookup" the executable).
@agocke
Copy link
Member Author

agocke commented Jul 15, 2022

@am11 Thanks!

@am11
Copy link
Member

am11 commented Jul 15, 2022

Ah, NO_CACHE was added in cmake v3.21 https://gitlab.kitware.com/cmake/cmake/-/merge_requests/6181/diffs#d8d61bf8b07dd46cb8a5399600b4c9512a6eba7f (I was testing it in v3.24). But we do support older version and some mono CI legs are using older cmake (which is good to have coverage).

We would need the old way of evicting the cached result. 😁

--- a/eng/native/configuretools.cmake
+++ b/eng/native/configuretools.cmake
@@ -28,8 +28,8 @@ if(NOT WIN32 AND NOT CLR_CMAKE_TARGET_BROWSER)
       return()
     endif()
 
+    unset(EXEC_LOCATION_${exec} CACHE)
     find_program(EXEC_LOCATION_${exec}
-      NO_CACHE
       NAMES
       "${TOOLSET_PREFIX}${exec}${CLR_CMAKE_COMPILER_FILE_NAME_VERSION}"
       "${TOOLSET_PREFIX}${exec}")

tested with cmake 3.10.

@agocke, if this PR is targeting 7.0, could you please apply this patch here. Otherwise I can open a new PR for this. Thanks!

@agocke
Copy link
Member Author

agocke commented Jul 15, 2022

Happy to apply it here.

@am11
Copy link
Member

am11 commented Jul 16, 2022

linux-musl failure is because the host machine is Ubuntu and build is running directly on the host rather than inside Alpine container.

@agocke agocke merged commit c4277b9 into dotnet:main Jul 18, 2022
@agocke agocke deleted the live-host branch July 18, 2022 18:27
agocke added a commit that referenced this pull request Jul 21, 2022
agocke added a commit that referenced this pull request Jul 21, 2022
agocke added a commit to agocke/runtime that referenced this pull request Jul 29, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 17, 2022
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.

6 participants