Skip to content

Use WARP from nuget by default #7427

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

damyanp
Copy link
Member

@damyanp damyanp commented May 3, 2025

Previously using WARP from nuget was something that had to be opted in to. This change makes it the default behvior.

This allows some simplifications. The nuget package is installed at configure time. If the user wants to override the nuget installation behavior then they can pass in extra parameters, rather than having an alternative mechanism for describing which package to use.

The actual DLL to load is specified through a TAEF parameter and specifies the full path to the DLL.

damyanp added 4 commits May 2, 2025 21:14
- remove special cases for warp
- move the generic sounding "nuget.cmake" closer to where it is used, and rename to DownloadWarp.cmake
- rearrange clang/test/CMakeLists.txt to make logic clearer about what is / isn't executed when using a multi-config generator
Copy link
Contributor

github-actions bot commented May 3, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@damyanp damyanp marked this pull request as ready for review May 3, 2025 19:00
@damyanp damyanp requested a review from alsepkow May 3, 2025 19:00
@damyanp damyanp requested a review from alsepkow May 9, 2025 21:56
Copy link
Contributor

@alsepkow alsepkow left a comment

Choose a reason for hiding this comment

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

LGTM, including the minor cast suggestion. Can re-approve if you change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was slightly surprised you didn't place this file under cmake/modules, not that I know much about cmake conventions. I was also thinking we should get the AgilitySDK from nuget as well, so I wonder if defining a general function for installing nugets would be even better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I now see that the original contained the general FetchNuGetPackage function, plus warp-specific code. Why wouldn't we want a general function defined under cmake/modules?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mainly because I don't want to spend time trying to design and build a generic function for installing nuget packages when we only have a need for this one. If we find a need for another, then we might be able to figure out what the common requirements between the two are and we can make an appropriate decision then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: New
Development

Successfully merging this pull request may close these issues.

3 participants