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

Move common settings from individual .csproj files to Directory.Build.props #1589

Open
manfred-brands opened this issue Jan 2, 2025 · 7 comments
Assignees

Comments

@manfred-brands
Copy link
Member

Items like OutputPath and AssemblyOriginatorKeyFile are necessary in every project.
We can reduce maintenance issues by moving these into Directory.Build.props

@manfred-brands manfred-brands self-assigned this Jan 2, 2025
@manfred-brands
Copy link
Member Author

After closer inspection there seems to be less common than expected.
@CharliePoole Is there a reason NUnit.Console and Nunit-Agents are not signed? Is that because of dynamic loading?

@CharliePoole
Copy link
Member

Partly for that reason, wrt the agents, but also because signing doesn't seem to offer any advantage for exe;s which are not referenced by other code. They could be, of course, although I see no advantage to signing the tests. How does the framework handle this nowadays?

WRT .props files, I've never been a big fan. Partly, it's just a matter of style. When I look at a project file, I like that all the settings are visible and I don't have to double-check one or more props files. Partly because, as a coach, I've been exposed to their use where people outside the dev team provide them, taking control away from the developers. Coming back to the engine after some time, however, I haven't wanted to change what the team put in place, so I retained the existing props file.

I'd favor only moving settings, which are required to be common to a props file. So, for output, I'd say that's not entirely common, TestData is handled differently. If we are using a key, then that should always be the same key but whether we are signing could vary. That said, if you think something will work better in the future when more people are hopefully working on this, please argue the case. :-)

@manfred-brands
Copy link
Member Author

For now I'm not going to argue my case as there are too many differences between the projects.

@CharliePoole
Copy link
Member

I'll take a look at this myself then.

@CharliePoole CharliePoole reopened this Jan 27, 2025
@CharliePoole
Copy link
Member

@manfred-brands Can you clarify the use of Condition="'$(ExposedInternals)' != 'true'" in Nullable.props for me? I can't find such a property in the docs for MSBuild.

@manfred-brands
Copy link
Member Author

manfred-brands commented Jan 27, 2025

The nuget packages Nullable and IsExternalInit declared their code (Attributes) as internal.
When a project referencing these has InternalsVisibleTo that code is also visible to those assemblies. If those assemblies also include these NuGet packages you will get duplicate symbol errors.

So this flag is to prevent that.

The flag is set in the test assemblies before importing nullable.props

@CharliePoole
Copy link
Member

I figured out part of that but was confused that you only added it to nunit.engine.core.tests. I see now that it only arises if you actually use one of the imported attributes like NullIfNotNull.

Right now I'm just making sure I understand stuff before I change it. I see that the framework's Directory.Build.props applies the same settings everywhere. Even the tests signed, for example. That may be the simplest thing as it does no harm.

Surprisingly, the adapter doesn't use nullability at all.

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

No branches or pull requests

2 participants