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

Use 'artifacts' instead of .artifacts for default artifacts path #31955

Merged
merged 2 commits into from
Apr 25, 2023

Conversation

dsplaisted
Copy link
Member

@dsplaisted dsplaisted commented Apr 21, 2023

Based on feedback from the .NET 8 Preview 3 support for artifacts output, this PR removes the . from the default artifacts output path.

It also blocks setting the artifacts path in the project file instead of in Directory.Build.props. Reasons for this include:

  • With the . removed, adding a new folder inside the project path would cause problems, since the new folder wouldn't be excluded from the default item includes
  • It would be unexpected that simply adding an empty Directory.Build.props file to your repo would move the artifacts path from the project folder up the tree to where the Directory.Build.props file was
  • Making it an error now might make it easier to change the heuristic in the future to look for .sln files, which might enable us to turn on the new output path format by default for some future TargetFramework.

Some things I still want to add to this PR:

  • Add an error if you set UseArtifactsOutput to true on the command line but don't have a Directory.Build.props file.
  • Add properties that allow you to opt in to putting the artifacts output in the project folder if you want, and to prevent the project name from being added to the path afterwards.
  • Verify that artifacts path is removed from globs

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Request triage from a team member label Apr 21, 2023
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@dsplaisted dsplaisted requested a review from a team April 25, 2023 17:28
Copy link
Member

@baronfel baronfel left a comment

Choose a reason for hiding this comment

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

Wording of the message and the . removal looks great to me 👍

@timheuer
Copy link
Member

I feel like we should also PR adding artifacts then to default .NET .gitignore files across the ecosystem then (similar to how dist is on default for JS apps

@baronfel
Copy link
Member

@timheuer it's already in the gitignore for the .NET CLI templates and the community-maintained ones at github/gitignore, so we should be covered.

@@ -896,4 +896,13 @@ You may need to build the project on another operating system or architecture, o
<value>NETSDK1198: A publish profile with the name '{0}' was not found in the project. Set the PublishProfile property to a valid file name.</value>
<comment>{StrBegin="NETSDK1198: "}</comment>
</data>
<data name="ArtifactsPathCannotBeSetInProject" xml:space="preserve">
<value>NETSDK1199: The ArtifactsPath and UseArtifactsOutput properties cannot be set in a project file, due to MSBuild ordering constraints. They must be set in a Directory.Build.props file or from the command line. See https://aka.ms/netsdk1199 for more information.</value>
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to manually lock the ArtifactsPath and UseArtifactsOutput strings inside a <note /> section for the translators so they cannot translate the property names by accident?

Copy link
Member Author

@dsplaisted dsplaisted Apr 25, 2023

Choose a reason for hiding this comment

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

Might be a good idea, can you point me to an example of what that looks like?

Copy link
Member

Choose a reason for hiding this comment

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

You can add something like:
<note>{Locked="dotnet workload search"}</note>
if dotnet workload search is what you want to lock.

This PR has a lot of examples: https://github.com/dotnet/sdk/pull/28553/files

Copy link
Member

@joeloff joeloff Apr 25, 2023

Choose a reason for hiding this comment

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

In the ENU resx file you should be able to add something like

<comment>{Locked="ArtifactsPath"} {Locked="UseArtifactsOutput"}</comment>

The comments will automatically be added in the XLF as notes for the translators IIRC. I can't remember if you need a single comment or multiple comments with separate locks. I think it's the former

@@ -74,6 +74,12 @@ Copyright (c) .NET Foundation. All rights reserved.
<BaseIntermediateOutputPath Condition="'$(BaseIntermediateOutputPath)' == ''">$(ArtifactsPath)\obj\</BaseIntermediateOutputPath>
</PropertyGroup>

<!-- Record whether ArtifactsPath / UseArtifactsOutput was set at this point in evaluation. We will generate an error if these properties are set
Copy link
Member

Choose a reason for hiding this comment

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

Should this be inside the PropertyGroup on line 67 (it has the same condition)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, but I will try to do that later since I'd like to get this in before we branch for Preview 4.

@dsplaisted dsplaisted merged commit 0091fb4 into dotnet:main Apr 25, 2023
@nagilson
Copy link
Member

I think this is a great change. With that said, this is technically a breaking change even though it only breaks one preview to preview version. Im not sure what our modus operandi here is but IMO we should create a breaking change doc, especially since this is removing a feature, we should do all we can to give clarity as to why we took something away and how to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-NetSDK untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants