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

Support feature settings on descriptor files #1277

Merged
merged 9 commits into from
Jun 23, 2020

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Jun 16, 2020

Fixes #1268.

@sbomer sbomer requested a review from marek-safar as a code owner June 16, 2020 22:50
@sbomer sbomer self-assigned this Jun 16, 2020
@sbomer sbomer force-pushed the featureDescriptors branch from bc8dff0 to 7b81c4a Compare June 16, 2020 23:21
@sbomer sbomer force-pushed the featureDescriptors branch from 7b81c4a to ae977ed Compare June 16, 2020 23:39
@vitek-karas vitek-karas requested a review from tlakollo June 17, 2020 14:23
@vitek-karas
Copy link
Member

@tlakollo Could you please double check that we don't "return" from the attribute XML parser in places where we should "continue" instead? Also would be nice to add a test to validate at least one of those cases.

@vitek-karas vitek-karas added this to the .NET5.0 milestone Jun 17, 2020
Since they conflict with the --exclude-feature "feature" attributes in
the descriptor XML.
Copy link
Contributor

@marek-safar marek-safar left a comment

Choose a reason for hiding this comment

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

Few minor things, otherwise LGTM

src/linker/Linker.Steps/ResolveFromXmlStep.cs Outdated Show resolved Hide resolved
src/linker/Linker.Steps/ResolveFromXmlStep.cs Outdated Show resolved Hide resolved
sbomer added 5 commits June 17, 2020 18:07
To indicate that a substitution or descriptor should be applied in the
absence of a feature setting. Note that this is restricted to still
require the featurevalue attribute, to prevent the definition of
default substitutions that are never applied for any feature settings.

Also address PR feedback:
- Move the feature checks into a separate class
- #ifdef an implementation instead of callsites
@sbomer sbomer requested a review from marek-safar June 19, 2020 22:17
@sbomer
Copy link
Member Author

sbomer commented Jun 19, 2020

@eerhardt I have added support for featuredefault="true" in 27bdc80 - PTAL

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

What happens in the case where a Substitutions.xml file doesn't specify a featuredefault, but a Descriptors.xml does for the same feature switch?

docs/error-codes.md Show resolved Hide resolved
@vitek-karas
Copy link
Member

What happens in the case where a Substitutions.xml file doesn't specify a featuredefault, but a Descriptors.xml does for the same feature switch?

Nothing interesting 😉 . Each element in the XML is independent. So if you want to have the same behavior between substitutions and descriptors, you'll need to specify the same thing twice.

@sbomer
Copy link
Member Author

sbomer commented Jun 22, 2020

What happens...

Sorry, I missed this question - thanks for answering it @vitek-karas

sbomer added 2 commits June 22, 2020 22:34
Add comments about order of checking assemblyname and feature attributes,
and make processing consistent in LinkAttributesStep.
@sbomer sbomer merged commit f87cfa2 into dotnet:master Jun 23, 2020
tkapin pushed a commit to tkapin/runtime that referenced this pull request Jan 31, 2023
* Support feature settings on descriptor files

Fixes dotnet/linker#1268.

* Disable feature descriptors for mono

Since they conflict with the --exclude-feature "feature" attributes in
the descriptor XML.

* Revert "Disable feature descriptors for mono"

This reverts commit dotnet/linker@78c2f52.

* Support featuredefault="true"

To indicate that a substitution or descriptor should be applied in the
absence of a feature setting. Note that this is restricted to still
require the featurevalue attribute, to prevent the definition of
default substitutions that are never applied for any feature settings.

Also address PR feedback:
- Move the feature checks into a separate class
- #ifdef an implementation instead of callsites

* Fix signatures in tests

* Update data format docs

* Fix whitespace

* PR feedback

Add comments about order of checking assemblyname and feature attributes,
and make processing consistent in LinkAttributesStep.

* Clarify featuredefault behavior in documentation

Commit migrated from dotnet/linker@f87cfa2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support feature conditions on XML descriptors
4 participants