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

Update Project Template #490

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Update Project Template #490

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a6a761
Update Project Template
robertmclaws Sep 15, 2024
c58ce5e
Fix Dacpac Build Properties
robertmclaws Sep 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions src/Microsoft.Build.Sql.Templates/sqlproject/SqlProject1.sqlproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,16 @@
<?xml version="1.0" encoding="utf-8"?>
<Project DefaultTargets="Build">
<Sdk Name="Microsoft.Build.Sql" Version="###ASSEMBLY_VERSION###" />
<PropertyGroup>
<Name>SqlProject1</Name>
<DSP>Microsoft.Data.Tools.Schema.Sql.{TargetPlatform}DatabaseSchemaProvider</DSP>
<ModelCollation>1033, CI</ModelCollation>
</PropertyGroup>
</Project>
<Project Sdk="Microsoft.Build.Sql/###ASSEMBLY_VERSION###" DefaultTargets="Build">

<PropertyGroup>
<SqlTargetName>SqlProject1</SqlTargetName>
Copy link
Member

Choose a reason for hiding this comment

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

The default for SqlTargetName is actually just $(MSBuildProjectName) so it does seem like the Name is not honored.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that is why I changed it. Changing the name did not change the filename at all. So instead of using a property that literally nothing else uses (Name), I bubbled up the existing property from SSDT.

<DacVersion>1.0.0.0<DacVersion>
Copy link
Member

Choose a reason for hiding this comment

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

This is already defined in Microsoft.Data.Tools.Schema.SqlTasks.targets. Is this just to add visibility to this property?

Copy link
Author

@robertmclaws robertmclaws Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's that changing the Name tag has zero effect on changing the file name, while setting this property changes both the Name in the Metadata of the Dacpac AND the filename.

The SDK.targets file either needs to use Name to set the SqlTargetName, or just use the SSDT targets property directly.

For DacVersion, yes it's in the Targets file but only if the version is not set. Setting <Version> in the project also has no effect on this property. So again, the SDK targets file either needs to take the Version and/or PackageVersion property and use those to set the DacVersion... or just let the user set the DacVersion directly and have THAT property set the others.

Either way, the only way right now for an end user to know these properties exist is to dig through the Microsoft.Data.Tools.Schema.SqlTasks.targets file in the NuGet package... which is not helpful.

<DSP>Microsoft.Data.Tools.Schema.Sql.{TargetPlatform}DatabaseSchemaProvider</DSP>
<ModelCollation>1033, CI</ModelCollation>
</PropertyGroup>

<!-- Correct the build in Visual Studio so it doesn't trigger a NuGet Framework Reference error.
<Target Name="PreBuild" BeforeTargets="PreBuildEvent" Condition="'$(MSBuildRuntimeType)' != 'Core'">
<Message Importance="high" Text="Ensuring the $(MSBuildThisFileDirectory)\obj\project.assets.json file is removed, if necessary, so that the database project can be built through VisualStudio SSDT without errors" />
<Delete Files="$(MSBuildThisFileDirectory)\obj\project.assets.json" />
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, this target would be triggered for every build. @Ri7Sh Do you see any issues with that?

Copy link
Contributor

Choose a reason for hiding this comment

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

@robertmclaws Can you please raise an issue describing the bug as well?

Copy link
Author

Choose a reason for hiding this comment

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

The code originally came from this post: #180 (comment) and fixes issue #180.

If you think it would be better in the SDK.targets file, I'm happy to do another check-in that moves it there.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah - clarifying in this thread - this doesn't fix support in VS, it fixes the specific case in VS when you try to build the project after having built it on the same machine with .NET 8 (cli or VS Code). This would be good to resolve @Ri7Sh but let's make sure we converge on an approach in the .NET template as well as in the VS template.

Copy link
Author

@robertmclaws robertmclaws Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@dzsquared I believe that is correct. For more context, if you build in VS the first time, it will build correctly.

But let's say you're trying to verify the build on the CLI so you're sure it will run in your CI/CD pipeline. So you run a dotnet build in the VS Terminal.

The next time you will build in Visual Studio, you will get the following error:

Your project does not reference ".NETFramework,Version=v4.7.2" framework. Add a reference to ".NETFramework,Version=v4.7.2" in the "TargetFrameworks" property of your project file and then re-run NuGet restore.

But that is not explicitly a .NET 8 problem, as this happened while I was trying to move to an SDK-style project in a .NET 9 RC2 app. I personally believe it is an issue in the downstream targets file that sneaks in a .NET Framework reference to get the compiler to work, which I believe is a mistake. But I can't find where to contribute a fix to that targets file.

Including this in the project template makes sure that error doesn't happen.

if you would like I can take this out of this PR and put it in a separate one... however this PR contained everything I needed to get a build working consistently both locally and in Azure DevOps, which is why this fix was also included here, so it could get into customer's hands ASAP during the preview, and you cold maybe move the fix somewhere more appropriate later.

HTH!

</Target>

</Project>
Loading