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 support for building S.P.CoreLib in a separate job #34166
Add support for building S.P.CoreLib in a separate job #34166
Changes from all commits
74cb414
def5d70
fc99faa
de8f012
34a19e8
eb1ee34
478b77f
a6a2a3b
fbfb35e
46efc4d
4fb3931
57b5d81
6436fb6
0683d82
f21a514
19e6701
6b73ed5
bb09589
2ec1f5a
77dfbcf
e6d4882
afe6717
a082f9c
fec325d
dfef8c3
2e4d00a
42dfd0d
56bf4c0
18e5190
e76db3a
e3c15b0
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.
WebAssembly is still depending on the full coreclr build. We should be able to also make it build against corelib by doing something similare as above.
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.
yup will do as a subsequent 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.
these are now included in this PR: #34460
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 believe
allConfigurations
shouldn't depend on the whole coreclr build. It should only need corelib as the tests we run there don't usetesthost
.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.
Disregard,
allConfigurations
nornet472
use this template at all. We can actually remove thein(parameters.framework
conditions above and just leave it as: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 will do as a subsequent 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.
these are now included in this PR: #34460
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 about instead directly invoking pretest.proj from CI and passing in the target that you care about?
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.
Yeah that makes sense. We could just call change the current call to /t: GenerateTestSharedFrameworkDepsFile.
Maybe on a follow-up PR so that he can merge?
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.
Yeah I'm fine with that.
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 will do on next 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.
This is CI only, why not directly invoking the runtime.depproj before tests.proj?
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.
one issue is that build parallelization might cause a race, pretest.proj has to complete before tests start to build, since pretest generates the dep file to ensure testhost is setup correctly.
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.
Not following. You can do two steps before the test build so the sequence would be:
Noteworthy, if you would wait until Monday, we could just invoke the lib-pretest lib-tests subsets which would handle all that for you. I plan to merge this on Monday (if ready) which introduces the libraries subsets #33553.
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 ideally want to merge this tomorrow, since this PR touches a bunch of infra pieces. I can commit to reworking this once your PR is merged.
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.
No problem at all, I can make the changes afterwards.