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
Add new Pipeline for Running Libs Tests with TestReadyToRun #91229
Add new Pipeline for Running Libs Tests with TestReadyToRun #91229
Changes from 50 commits
58471ac
d424419
cbae83d
bbcbda5
74c393d
f39ec24
e76d7b2
800bf85
4888506
f95be94
ce0beb7
084dcd2
d6145d9
7a98a59
57a1ed0
9c9845c
201c744
0e5318e
94b209d
baf03ff
bd083f9
70a5310
29bdc40
0dafa7a
0987abf
c050621
21f95ef
59af6bb
52875a7
43eee05
4666ac0
79a9028
e95e20d
e857281
d193ab5
7a3a1d5
cd94d5a
3c54a84
879e5ea
11cdb6b
19bb85f
81cb9e3
47760b3
0728170
4262460
403a5f9
797f2c0
048d35e
9939472
033508d
54088ef
7a23788
7c97aa7
f54af82
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.
Please describe these targets via xml comments. I don't understand what the following targets are doing or why we need them so a comment would be appreciated:
ExcludeExistingR2RBinaries
,AddExistingR2RBinariesReferencesForCrossgen2
,CopyExistingR2RBinaries
andCopyLiveRefPackIfPresent
.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.
Agree with @ViktorHofer here - copies like this can cause extra work and even create incremental build issues when they use wildcards like this. It'd be good to understand them.
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.
There's zero references to TestReadyToRun in src/libraries. Should all of TestReadyToRun handling stay in eng/? tests.singlefile.targets looks more suitable.
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.
I tried moving it, but then the build fails in other places because it can't find the
apphost
. Like for example, this one:In this case, this means
UseLocalAppHostPack
is set to false, i.e., it is not using it when building the tests and that's where we need it. We don't need it forclr
orlibs
, justlibs.tests
. So I'd like to ask if you had another possible solution, if adding it toDirectory.Build.props
is not right.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.
You can use the
PublishToDisk
target with theOutputPath
property pointing to where you want the project dumped on disk on just the bundleproj and that will dump the whole layout to disk with R2R'd runtime assemblies.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.
Why is that required? I see that workload tests only have the ProjectReference with the
Pack
metadata.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.
I'm not sure how workload tests work, but I wouldn't be amazed if they need the NuGet packages or the installers or the archives.
This is another option that I built into the Shared Framework SDK to make it easier to dump the layout to disk wherever you want and not having to guess where the output will be.
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.
Got it. What's the default output location of the bundle project? Is it put to disk somewhere without invoking the target?
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.
The Shared Framework SDK by default doesn't actually lay it out on disk. The Archives and Installers NuGet packages from dotnet/arcade use this target to put it into the intermediate output path for the project and then use that to produce their output.
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.
Ok. For the sake of static graph (which disallows calling custom targets in a P2P protocol), can we enable a switch so that the target is automatically sequenced into the build? The project's output directory would be a good default place.
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 should be able to add the target to the Bundle.bundleproj project and sequence it into the build, but then we still end up building the installers (which is what's causing the build failures that prompted this suggestion).