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

Jump to bottom

Allow adding any property to a VisualStudio project #2112

Merged
merged 1 commit into from
Dec 5, 2023
Merged

Allow adding any property to a VisualStudio project #2112

merged 1 commit into from
Dec 5, 2023

Conversation

hanagasira
Copy link
Contributor

What does this PR do?

At #2025 I was able to use the nuget package from Microsoft.Direct3D.D3D12.

But to parameterize this package, we need to set properties such as Microsoft_Direct3D_D3D12_D3D12SDKPath and Microsoft_Direct3D_D3D12_SkipLibraryCopy etc.

Also in charp, we need to set properties for nullable and other settings.

Instead of creating dedicated functions for each of these settings, I would like to create a single function that can set any property.

How does this PR change Premake's behavior?

Add a props command to allow setting arbitrary properties

Anything else we should know?

no

Did you check all the boxes?

  • Focus on a single fix or feature; remove any unrelated formatting or code changes
  • Add unit tests showing fix or feature works; all tests pass
  • Mention any related issues (put closes #XXXX in comment to auto-close issue when PR is merged)
  • Follow our coding conventions
  • Minimize the number of commits
  • Align documentation to your changes

You can now support Premake on our OpenCollective. Your contributions help us spend more time responding to requests like these!

Jarod42
Jarod42 reviewed Jul 27, 2023
View reviewed changes
modules/vstudio/vs2005_dotnetbase.lua Outdated
function dotnetbase.additionalProps(cfg)
for i = 1, #cfg.props do
for key, value in spairs(cfg.props[i]) do
_p(2, '<%s>%s</%s>', key, value, key)
Copy link
Contributor

@Jarod42 Jarod42 Jul 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Wonder if we have a xml escaper for value...
It seems not according to above function (which is more likely to have < or >.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are multiple xml escaping functions, which one should I use?
For now, I use vs2005.esc for csharp, vs2010.esc for c++

src/_premake_init.lua Outdated Show resolved Hide resolved
website/docs/props.md Outdated Show resolved Hide resolved
website/docs/props.md Outdated
Add any property to your visual studio project
This allows you to set properties that premake does not support without extending it

Values set at one time are sorted alphabetically
Copy link
Contributor

@Jarod42 Jarod42 Jul 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

if order is important for msvc, adding in the doc the way to specify order (several calls) should be added IMO.
else that sentence can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it to the document because it is sometimes necessary to specify the order when reusing properties defined before.
By the way, the reason for sorting by name is that the order of the lua table is not always the same.

src/_premake_init.lua Outdated
@@ -1062,6 +1062,13 @@
kind = "list:table",
}

api.register {
name = "props",
Copy link
Contributor

@Jarod42 Jarod42 Jul 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if the name should not reflect that it is visual specific (vsprops)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to vsprops

Copy link
Member

Choose a reason for hiding this comment

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

Late to the party, but can this life in modules/vstudio/_preload.lua instead, since it's specific to VS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. I have moved the definition to modules/vstudio/_preload.lua.

@hanagasira hanagasira force-pushed the add_vs_props branch 2 times, most recently from 0dfc5f5 to 67de2d1 Compare July 29, 2023 13:21
@hanagasira hanagasira marked this pull request as draft July 29, 2023 13:30
@hanagasira hanagasira force-pushed the add_vs_props branch 3 times, most recently from 2221460 to 9b9f3ca Compare July 31, 2023 13:39
@hanagasira hanagasira marked this pull request as ready for review July 31, 2023 13:41
@hanagasira hanagasira force-pushed the add_vs_props branch 2 times, most recently from e957529 to e79b1d8 Compare August 15, 2023 12:05
@nickclark2016
Copy link
Member

I think this needs to be added in the sidebar still. Other than that, I think I'm happy with the code (though a merge conflict needs to be resolved, if you could do that, it would be very appreciated).

@hanagasira
Copy link
Contributor Author

@nickclark2016
I rebased and resolved the conflict.

@KyrietS
Copy link
Member

KyrietS commented Oct 31, 2023

Please add vsprops to the sidebar: https://github.com/premake/premake-core/blob/master/website/sidebars.js#L293
Some day when we upgrade Docusaurus to the latest version we might have auto-generated sidebar, but for now we need to maintain it manually 🙃

@Jarod42
Copy link
Contributor

Jarod42 commented Oct 31, 2023

premake-core/website/sidebars.js

Lines 291 to 294 in 54af123

'visibility',
'vpaths',
'warnings',
'workspace',

->

 'visibility', 
 'vpaths', 
 'vsprops',
 'warnings', 
 'workspace',

@hanagasira
Copy link
Contributor Author

@KyrietS @Jarod42
Thanks for the explanation. I have recorrected it.

@nickclark2016 nickclark2016 merged commit e3855de into premake:master Dec 5, 2023
14 checks passed
@hanagasira hanagasira deleted the add_vs_props branch December 6, 2023 11:50
@theComputeKid theComputeKid mentioned this pull request Jun 1, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jarod42 Jarod42 Jarod42 left review comments

@nickclark2016 nickclark2016 nickclark2016 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hanagasira @nickclark2016 @KyrietS @Jarod42