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

Removed all references to $(SolutionDir) from build artifacts #1894

Merged
merged 3 commits into from Oct 16, 2023

Conversation

JeromyWalsh
Copy link
Collaborator

@JeromyWalsh JeromyWalsh commented Oct 5, 2023

PR Details

Removed all references to SolutionDir or replaced them with references to $(MSBuildThisFileDirectory).

Description

The $(SolutionDir) property is not generally regarded as an MSBuild-defined property and is instead understood to be a Visual Studio property. Microsoft has made it clear by dropping support for the -r flag from "dotnet build" on solution files, among other things, that the unit of work for command-line and automated builds should be the .csproj/.vcxproj files, not the .sln file.

This change replaces any references to $(SolutionDir) with the appropriate alternatives, or removes it outright when unnecessary.

Motivation and Context

This is a future-forward change that not only untangles an unnecessary, tool-dependent property, but lowers maintenance costs and moves us one step forward towards building on other platforms and outside of Visual Studio or other tools that require a .sln file.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Removed all references to SolutionDir or replaced with references to $(MSBuildThisFileDirectory).
@@ -27,7 +27,7 @@

<!-- Add support for building this project from the Project Directory -->
<PropertyGroup>
<SolutionDir Condition="'$(SolutionDir)' == ''">$(ProjectDir)..\..\..\build\</SolutionDir>
<DependencyDir>$(ProjectDir)..\..\..\..\deps</DependencyDir>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a little nitpick:
Shouldn't it be DependenciesDir instead? It's the directory where all Stride dependencies are located.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @Ethereal77, grammatically speaking you're not wrong, and if people have a strong preference it's an easy change to make.

That being said, historically the preference has been for readability/laziness rather than grammatical accuracy. ;-) As examples, there used to be the "src" and "source", folders. Still remaining are the "bin", and "obj" folders. In our own project hierarchy there's the "build" folder, though technically it's the "builds" folder. Those are all cases where the folder name acted as a sort/filter rather than as a description of the contents, ie. "this is where a source file goes," or "this is where a build file goes."

I also just found DependecyDir easier to read and say than DependenciesDir. But again, that's likely just a dialectical preference or laziness. If people prefer the latter, I'm happy to change it. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a strong preference. I'm not a native english speaker, so it doesn't matter much to me. Anyway I agree with you on the simplicity point.
Readability > all else 😁

@Eideren
Copy link
Collaborator

Eideren commented Oct 7, 2023

Will merge this one once #1694 is merged to avoid more merge conflicts over there.
Might introduce conflicts on this one but we'll see.

@JeromyWalsh
Copy link
Collaborator Author

Will merge this one once #1694 is merged to avoid more merge conflicts over there. Might introduce conflicts on this one but we'll see.

Dude, you don't need to merge my PR for me. :-) You do enough already. I'll merge it and handle resolving the merge conflicts.

@Eideren
Copy link
Collaborator

Eideren commented Oct 7, 2023

Ahah, thanks. This was also to let other maintainers know that they shouldn't merge it yet if they happen to pass by :)

@Eideren Eideren merged commit 0bc67fc into stride3d:master Oct 16, 2023
1 of 14 checks passed
@Eideren
Copy link
Collaborator

Eideren commented Oct 16, 2023

Thanks !

@MaximilianEmel MaximilianEmel mentioned this pull request Oct 17, 2023
7 tasks
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.

None yet

4 participants