Skip to content

Add a test for attribute removal using wildcard and lazy loading#1705

Merged
vitek-karas merged 2 commits into
dotnet:masterfrom
vitek-karas:AttributeRemovalOnLazyLoaded
Dec 17, 2020
Merged

Add a test for attribute removal using wildcard and lazy loading#1705
vitek-karas merged 2 commits into
dotnet:masterfrom
vitek-karas:AttributeRemovalOnLazyLoaded

Conversation

@vitek-karas
Copy link
Copy Markdown
Member

Currently this doesn't work as it should, the test is to cover the scenario - once we fully implement lazy loading this should be fixed.

Currently this doesn't work as it should, the test is to cover the scenario - once we fully implement lazy loading this should be fixed.
@sbomer
Copy link
Copy Markdown
Member

sbomer commented Dec 17, 2020

Didn't we decide that the wildcard was only supported for corelib? #1675 intentionally removed support for * in embedded attribute XML except corelib. I would expect that the attribute isn't removed in this case, with or without lazy loading.

@vitek-karas
Copy link
Copy Markdown
Member Author

Good point - that basically means there's no way to write a test for it, right? (I can't fake corelib)
But the functionality is still broken...
Should we maybe add some test hook to enable it on other assemblies so that it can be tested?

Also - this pretty much means that the LinkerAttributeRemoval test should not work, right? So how come it's passing?

@sbomer
Copy link
Copy Markdown
Member

sbomer commented Dec 17, 2020

It's a bit of a hack, but I managed to write a test that mocks corelib: https://github.com/mono/linker/pull/1675/files#diff-6bc3fb243979b37206fe4be289fcc1c77b1b6ec8173e7e63a090603639dd7c75R20. It fails PeVerify because system types can't be resolved, but it does check the attribute removal in other assemblies.

As to why the test is passing - I just noticed it's actually not using embedded xml. :) SetupLinkAttributesFile will use the command-line xml.

@vitek-karas
Copy link
Copy Markdown
Member Author

Good point about the file not being embedded - in that case the test is perfectly valid, right?

@sbomer
Copy link
Copy Markdown
Member

sbomer commented Dec 17, 2020

Right, good point. And it's a good test to have, since I have been focusing mostly on the embedded XML. The "embedded" part threw me off. :)

@vitek-karas vitek-karas merged commit f4c577e into dotnet:master Dec 17, 2020
@vitek-karas vitek-karas deleted the AttributeRemovalOnLazyLoaded branch December 17, 2020 21:41
agocke pushed a commit to dotnet/runtime that referenced this pull request Nov 16, 2022
…net/linker#1705)

Currently this doesn't work as it should, the test is to cover the scenario - once we fully implement lazy loading this should be fixed.

Commit migrated from dotnet/linker@f4c577e
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.

3 participants