-
Notifications
You must be signed in to change notification settings - Fork 354
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
Fix some workflow issues #2720
Fix some workflow issues #2720
Conversation
am11
commented
Oct 30, 2021
- idempotently generate version file.
- allow double hyphen in args.
ce8dfc0
to
2768d81
Compare
eval "$__RepoRootDir/eng/native/version/copy_version_files.sh" | ||
if [[ ! -e "$__RepoRootDir/artifacts/obj/_version.c" ]]; then | ||
eval "$__RepoRootDir/eng/native/version/copy_version_files.sh" | ||
fi |
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.
@jkoritzinsky, in official build, we always build from clean repo so I assumed this won't be a problem.
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.
Yes we always build from a clean clone for an official build.
Is this PR ready to go? It looks fine to me. |
This and dotnet/runtime#61027 are mirror PRs. Ready for review. @hoyosjs, could you please take a look? As a next step, we can look into moving bits from eng/native to eng/common/native to nail out code duplication across repos, which make use of native infra. |
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.
Sorry, thought I'd commented here that I was going to complete the fix last week. Time just didn't allow. The PR does solve an issue of the incrementality somewhat, I'm fine with it going as is. However, it doesn't solve some of the issues that were introduced in our infra. The runtime uses versioning.targets
and it's connected in no way in this repo. This means SOS ends up with versioning info like this in our official builds:
@(#)Version N/A @Commit: a3464a7
I'll likely just add a call in build.ps1/sh to an msbuild phony target to generate the files.
I remember testing it with |
@am11 yup, and it's not a big deal. I was just going to bolt on the fix. I just ended up doing a fairly rabbit-hole investigation and ended up not doing it last week. That commit that you pointed out handles it in runtime.proj. We don't have an equivalent entrypoint to native on this project and we don't have the subsets, hence I said most likely just a call into empty.proj with an explicit target call. I'll probably do it with another versioning change I have to do. |