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

nuget_dependency: Allow path override #871

Merged

Conversation

Javagedes
Copy link
Contributor

@Javagedes Javagedes commented Aug 22, 2024

As described in external_dependency.py#L38, the name configuration is the name of the ext_dep, used to name the folder the ext_dep will be unpacked into. When the nuget external dependency was added, name also became the name of the package you are downloading, which tightly couples the name of the package and the folder path. This means there is no way to name the folder something other than the package name, which can be necessary in some situations.

This change provides a backwards compatible way to de-couple the package name from the folder name by introducing a new, optional config for nuget dependencies, package. package will be used as the package name to download while name will continue to be the name of the ext dep and the folder it is downloaded to. For backwards compatability, if package is not used in the configuration file, name will be used instead.

@Javagedes Javagedes requested review from makubacki and apop5 August 22, 2024 19:52
@Javagedes Javagedes added the enhancement New feature or request label Aug 22, 2024
@Javagedes Javagedes self-assigned this Aug 22, 2024
@Javagedes Javagedes added this to the v0.27.12 milestone Aug 22, 2024
@apop5
Copy link

apop5 commented Aug 22, 2024

Is there an example for this?

@Javagedes
Copy link
Contributor Author

Is there an example for this?

@apop5 The example that brought about this requested change is header files that are downloaded via external dependency and are in a package's Include/* folder. There are two mutually exclusive external dependencies at this location, but only one exists at any point in time. In that package's dec file, you cannot simply add both folders to it, because one will exist and one will not, and the build will fail. So they need to be able to download one or the other extdep to the same folder.

Additionally, macros don't work in dec files, so we cannot set a build variable within the extedep then do something along the lines of:

[Includes]
$(VAR_NAME)

@Javagedes Javagedes force-pushed the personal/joeyvagedes/add_package_name branch from 129b301 to 6cc65c7 Compare August 22, 2024 20:13
Python 3.12.5 is resulting in a documentation build failure due to
compatability issues with mkdocstrings. Once a fix has been released, we
will un-restrict the version.
@Javagedes Javagedes force-pushed the personal/joeyvagedes/add_package_name branch from 6cc65c7 to 767959e Compare August 28, 2024 15:05
Copy link

codecov bot commented Aug 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.71%. Comparing base (bf643b0) to head (767959e).
Report is 100 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #871      +/-   ##
==========================================
- Coverage   78.75%   78.71%   -0.05%     
==========================================
  Files          49       41       -8     
  Lines        4909     5050     +141     
==========================================
+ Hits         3866     3975     +109     
- Misses       1043     1075      +32     
Files with missing lines Coverage Δ
...oolext/environment/extdeptypes/nuget_dependency.py 87.50% <100.00%> (ø)

... and 48 files with indirect coverage changes

@Javagedes Javagedes merged commit 60ff370 into tianocore:master Aug 28, 2024
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants