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.

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

Miscellaneous refactors in Common props, targets and tasks #6161

Closed
wants to merge 3 commits into from
Closed

Miscellaneous refactors in Common props, targets and tasks #6161

wants to merge 3 commits into from

Conversation

Nirmal4G
Copy link
Contributor

@Nirmal4G Nirmal4G commented Feb 16, 2021

Fixes #TBA

Context

Make Common props, targets and tasks easier to read and understand.

Changes Made

Common changes in all files

Fix improper leading and trailing spacing of strings within quotes.

Microsoft.Common.props
  • Move 'BaseIntermediateOutputPath' logic out of 'MSBuildProjectExtensionsPath' block.
Microsoft.Common.CurrentVersion.targets
  • Add single quotes to property names in text.
  • Set ProjectPath to the now available MSBuildProjectDirectory.
  • Simplified condition logic wherever based on OutputType property.
  • Use ConfigurationName property instead of Configuration property.

TBA

Testing

NIL

Notes

I'll make sure that there's no functional changes in these refactors. If they do have, I'll create a separate PR for those.
I'll also separate the commits by common refactors and have a last one or two commits containing everything else to make reviewing easier.

For now, I'm placing this in Draft since there are many refactors to be added.

Please hold up your reviews until it's out of draft.

Base automatically changed from master to main March 15, 2021 20:09
@Nirmal4G Nirmal4G changed the base branch from main to vs16.10 May 13, 2021 11:40
@Nirmal4G Nirmal4G changed the base branch from vs16.10 to vs16.11 May 29, 2021 08:05
@Nirmal4G Nirmal4G changed the base branch from vs16.11 to main July 13, 2021 17:06
ladipro pushed a commit that referenced this pull request Jul 20, 2021
Fixes #1062

### Context

The import based on this property was introduced as a temporary bootstrapping
mechanism before there was packaging extensibility available for multi-targeting.

The packaging mechanism (aka NuGet) now uses `buildCrossTargeting`/`buildMultiTargeting`,
similar to `build` package folder to hold and import multi-targeting logic via NuGet's Restore.

Thus, we don't need this workaround anymore.


### Changes Made

Removed the temporary import based on `CoreCrossTargetingTargetsPath`


### Testing

Since there are no tests for this property, as it is with all the workarounds… We'll see if we break anyone during the self-hosting period.


### Notes

This patch was already a part of #6161. Since, the change was approved independently, I have separated this into a new PR.
Make Common props, targets and tasks easier to read and understand.

Ensure they follow consistent formatting

E.g.: 2-space indent
```xml
  <!-- Single Line Comment Text -->
  <!--
    Multi Line Comment Text
    Another Comment Text
      Indented Comment Text
  -->
```
Make Common props, targets and tasks easier to read and understand.

in all files:
 - Fix improper leading and trailing spacing of strings within quotes.

in 'Microsoft.Common.props':
 - Move 'BaseIntermediateOutputPath' logic out of 'MSBuildProjectExtensionsPath' block.

in 'Microsoft.Common.CrossTargeting.targets':
 - Remove temporary import depending on 'CoreCrossTargetingTargetsPath' property.

in 'Microsoft.Common.CurrentVersion.targets':
 - Add single quotes to property names in text.
 - Set 'ProjectPath' to the now available 'MSBuildProjectDirectory'.
 - Simplified condition logic wherever based on 'OutputType' property.
 - Use 'ConfigurationName' property instead of 'Configuration' property.
@Forgind
Copy link
Member

Forgind commented Apr 21, 2022

@Nirmal4G,

Some of your PRs have been open (and drafts) for a very long time. Are you still working on any of them? Would you mind if I close some?

@Nirmal4G
Copy link
Contributor Author

Yes, I'm and I have pushed my commits but not reflecting latest changes. I don't know why! Some of them are finished too but the latest changes are not reflecting.

@Forgind
Copy link
Member

Forgind commented Apr 21, 2022

What did you push to? I looked at the branch for this PR in your repo, and it seems to be 3 commits ahead of main, just as this PR is.

@Nirmal4G
Copy link
Contributor Author

Nirmal4G commented Apr 21, 2022

repository is deleted

You can see from the above image; the branch has been struck out with a tooltip saying "repository is deleted". I suspect it's because of re-forking MSBuild repo I did due to dependabot issues.

@Nirmal4G
Copy link
Contributor Author

@Forgind Closing this since I re-forked the repo, the branch ref to this PR was removed as well. I'll open a new PR soon. BTW PR 7168 has been completed, can you look into why it hasn't been merged all this time?

@Nirmal4G Nirmal4G closed this Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants