Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Dllimport generator build and test fixes #59658
Dllimport generator build and test fixes #59658
Changes from 30 commits
ea54b1f
6f40531
dbc141d
2a65756
a24525e
7a3148f
fe101e4
ed44881
e05c631
e5652ae
986e9a5
102a95d
4644434
ac37572
027ad10
d5e66f1
f77cd70
cf6e22b
a3efde9
c8fcd84
e77b7f0
e320cbd
d4aff51
392ee65
d633dc8
6107024
ff049ab
50c189e
6646f12
4d55f3a
2c94662
00726aa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is going to be the recommended way to make these work with GeneratedDllImports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular P/Invoke took a dependency on the fact that chars are conditionally blittable, so no copying was done. So far, the Interop team has decided to not consider
char
s as conditionally blittable.To make this particular P/Invoke work, there are a few different options:
char
parameters as blittable whenCharSet=CharSet.Unicode
.char*
parameters instead ofref char
and pass in pinned refs.string
(which will be pinned) or aReadOnlySpan<char>
(using the new Span marshalers) and the second to take achar*
, which will be passed without change. As the usage of this method calculates the refs to pass in, from aReadOnlySpan<char>
and aValueStringBuilder
, this would be possible without too much trouble.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We did (1) in #59700, so we should be able to switch this and
GetLongPathNameW
back.Fine with getting this change in to the feature branch and updating in the main merge PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the merge conflicts I had after the property order normalization, I just fixed this as part of resolving those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do this via a
ProjectExclusions
based onRuntimeFlavor
intests.proj
? The assembly attribute results in us still sending the test to Helix, running through test discovery for each test to determine that it should not be run, and uploading logs - which seems rather wasteful.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh. I completely forgot about that. +1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't do that because we don't build the libraries tests for each runtime flavor, we use one build for both CoreCLR and Mono on desktop platforms (like Linux and Mac). I tried that originally and it didn't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Learned about a new task today - not sure I'm happy about it...