Skip to content

Consolidate RID and native file naming in MSBuild scripts - #43804

Merged
3 commits merged into
dotnet:masterfrom
am11:feature/consolidation/msbuild-properties
Nov 19, 2020
Merged

Consolidate RID and native file naming in MSBuild scripts#43804
3 commits merged into
dotnet:masterfrom
am11:feature/consolidation/msbuild-properties

Conversation

@am11

@am11 am11 commented Oct 25, 2020 •
edited
Loading

Copy link
Copy Markdown
Member
  • 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
Comment thread eng/Configurations.props Outdated
@am11 am11 mentioned this pull request Nov 4, 2020
Merged
@am11

am11 commented Nov 5, 2020

Copy link
Copy Markdown
Member Author

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

@ViktorHofer

ViktorHofer commented Nov 12, 2020

Copy link
Copy Markdown
Member

@jkoritzinsky kindly asking for a review

@jkoritzinsky

jkoritzinsky commented Nov 12, 2020

Copy link
Copy Markdown
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 requested a review from marek-safar as a code owner November 13, 2020 15:53
@am11 am11 mentioned this pull request Nov 14, 2020
Closed
@ViktorHofer

ViktorHofer commented Nov 15, 2020

Copy link
Copy Markdown
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

@jkoritzinsky jkoritzinsky left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/installer/Directory.Build.props Outdated
Comment thread src/installer/Directory.Build.props Outdated
Comment thread src/tests/build.cmd
@am11

am11 commented Nov 19, 2020

Copy link
Copy Markdown
Member Author

windows x86 failure is #43389.

@runfoapp runfoapp Bot mentioned this pull request Nov 19, 2020
Closed
@jkoritzinsky

jkoritzinsky commented Nov 19, 2020

Copy link
Copy Markdown
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

ghost commented Nov 19, 2020

Copy link
Copy Markdown

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-ReadyToRun

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