Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Consolidate RID and native file naming in MSBuild scripts #43804

Merged
3 commits merged into from
Nov 19, 2020
Merged

Consolidate RID and native file naming in MSBuild scripts #43804

3 commits merged into from
Nov 19, 2020

Conversation

am11
Copy link
Member

@am11 am11 commented Oct 25, 2020 •
edited
Loading

  • Use short variable names for native files naming convention, that are
    used by framework.sharedfx.targets in arcade, and cleanup
    redefinitions from crossgen2 and installer. e.g. ExeSuffix instead
    of ApplicationFileExtension, LibSuffix instead of
    LibraryFileExtension and so on.
  • Calculate TargetArchitecture, NonPortableRuntimeOS (for
    PortableBuild) and PackageRID values once for the entire
    livebuild, inside eng/Configurations.props. This implementation is
    a union of three varied implementations that are being deleted.
  • Import names.props once in eng/Configurations.props based on
    calculated PackageRID and cleanup imports of this file from various
    places.
  • Combine OS targets definition in MSBuild scripts.

@am11 am11 marked this pull request as ready for review October 27, 2020 05:01
am11
am11 commented Oct 27, 2020
View reviewed changes
eng/Configurations.props Outdated Show resolved Hide resolved
@am11 am11 mentioned this pull request Nov 4, 2020
Merged
@am11
Copy link
Member Author

am11 commented Nov 5, 2020

cc @ViktorHofer, @jkoritzinsky, @janvorli,
PTAL, resolved merge conflicts and rebased. Both CI crashes in System.Runtime.Serialization.Formatters.Tests are #44250.

@ViktorHofer
Copy link
Member

@jkoritzinsky kindly asking for a review

@jkoritzinsky
Copy link
Member

I will review this as soon as my shared framework PR is merged in. I think there will likely be conflicts between the two PRs that will have to be resolved, but I'll do a pass though just to be sure.

@am11 am11 mentioned this pull request Nov 14, 2020
Closed
@ViktorHofer
Copy link
Member

Thanks a lot @am11. Jeremy will take a look soon :)

@am11
Consolidate RID and native file naming in MSBuild scripts
534c299 
* Use short variable names for native files naming convention, that are
  used by `framework.sharedfx.targets` in arcade, and cleanup
  redefinitions from crossgen2 and installer. e.g. `ExeSuffix` instead
  of `ApplicationFileExtension`, `LibSuffix` instead of
  `LibraryFileExtension` and so on.
* Calculate `TargetArchitecture`, `NonPortableRuntimeOS` (for
  `PortableBuild`) and `PackageRID` values once for the entire
  livebuild, inside `eng/Configurations.props`. This implementation is
  a union of three varied implementations that are being deleted.
* Import `names.props` once in `eng/Configurations.props` based on
  calculated `PackageRID` and cleanup imports of this file from various
  places.
* Combine OS targets definition in MSBuild scripts.
jkoritzinsky
jkoritzinsky approved these changes Nov 19, 2020
View reviewed changes
Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few comments around a few more things that can be deleted.

Other than that, LGTM if CI is green.

src/installer/Directory.Build.props Outdated Show resolved Hide resolved
src/installer/Directory.Build.props Outdated Show resolved Hide resolved
src/tests/build.cmd Show resolved Hide resolved
@am11
Copy link
Member Author

am11 commented Nov 19, 2020

windows x86 failure is #43389.

@runfoapp runfoapp bot mentioned this pull request Nov 19, 2020
Closed
@jkoritzinsky
Copy link
Member

I'm rerunning the failed job just to validate that the failure was a flaky test. Once that's green, I'll merge.

@ghost
Copy link

ghost commented Nov 19, 2020

Hello @jkoritzinsky!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 75c0b99 into dotnet:master Nov 19, 2020
@am11 am11 deleted the feature/consolidation/msbuild-properties branch November 19, 2020 18:52
This was referenced Nov 20, 2020
Merged
Merged
@MattGal MattGal mentioned this pull request Nov 20, 2020
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 19, 2020
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dagood dagood dagood left review comments

@jkoritzinsky jkoritzinsky jkoritzinsky approved these changes

@marek-safar marek-safar Awaiting requested review from marek-safar

Assignees
No one assigned
Labels
area-crossgen2-coreclr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@am11 @ViktorHofer @jkoritzinsky @dagood @Dotnet-GitSync-Bot