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

Add build steps to Visual Studio #1403

Merged
merged 2 commits into from
Mar 5, 2020
Merged

Add build steps to Visual Studio #1403

merged 2 commits into from
Mar 5, 2020

Conversation

redorav
Copy link
Collaborator

@redorav redorav commented Feb 17, 2020

What does this PR do?

Adds build steps to Visual Studio through the buildcommands interface. (edit: See issue #1400 for more information and discussion).

How does this PR change Premake's behavior?

This adds missing functionality, to be able to have build steps with custom inputs and outputs

Anything else we should know?

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

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

@redorav redorav mentioned this pull request Feb 17, 2020
@starkos
Copy link
Member

starkos commented Feb 22, 2020

I haven't tried, but I suspect that this will encounter the same bug I just fixed in #1401. Premake allows this:

project "MyProject"
   buildcommands { "cmd.exe %{cfg.buildcfg.abspath}" }

If you're trying to process that command at the project level, it will fail because cfg will not be set. See #1013 for more discussion.

@redorav
Copy link
Collaborator Author

redorav commented Feb 22, 2020

Sorry, scratch what I said last. My command is something like the following:

project "MyProject"
buildcommands
{
"\"%{cfg.buildtarget.directory}"..ExecutableName..".exe\""
}

This puts the following in the build steps command line:

"$(TargetDir)ExecutableName.exe"

So it seems to have replaced it properly in this instance. However, testing {cfg.buildcfg.abspath} will output an empty string, even within a configuration block. Is this correct behavior?

Update: Tokens such as %{wks.name}, %{prj.location} and %{cfg.targetdir} all get replaced properly.

@starkos
Copy link
Member

starkos commented Feb 22, 2020

I'd be curious what %{cfg.targetdir} emits in that situation.

The relevant condundrum is: Premake allows you to specify settings in a "higher" scope (global, workspace, project) that get inherited by lower or narrower scopes (configuration, file). So when I use script like…

project "MyProject"
   buildcommands { "cmd.exe %{cfg.targetdir}" }

…my intention is that a separate command would be created for each configuration (think "Debug", "Release") in my project. I would expect that my project's debug configuration to use cmd.exe bin/Debug, for example, and release to use cmd.exe bin/Release.

I'm not sure what the right answer would be here. A new set of APIs might the easiest, but I'm open to more general-purpose suggestions.

@redorav
Copy link
Collaborator Author

redorav commented Feb 22, 2020

As far as I can tell, this does the right thing in the case you describe. I have come up with the following:

configuration("Debug")
	targetdir ("MyTargetDir1")
		
configuration("Release")
	targetdir ("MyTargetDir2")

configuration {} -- Reset configurations so it applies to all

buildcommands
{
	"%{cfg.targetdir}"
}

And this expands the build command to ../../MyTargetDir1 only for Debug, and only MyTargetDir2 respectively on the Release configuration. It also respect configuration filters, so if I was to not have th configuration {} line, it only applies to Release. It seems to me like in general things work as expected. Is there perhaps a bug with specific tokens?

@starkos
Copy link
Member

starkos commented Feb 26, 2020

Thanks for doing that research, I think this looks workable. If you could add some unit tests and squash the commits I think it should be good to approve.

Thanks for contribution!

@redorav
Copy link
Collaborator Author

redorav commented Feb 27, 2020

I've added the unit tests and tried rebasing/squashing, but I'm making a mess like I did last time I tried doing the process. I think I conceptually understand what I'm trying to do, I just don't get how to do it with git...

@redorav redorav closed this Feb 27, 2020
@redorav redorav reopened this Feb 27, 2020
@redorav
Copy link
Collaborator Author

redorav commented Feb 27, 2020

I've kind of done it in a brute force way by rewinding the history and re-pushing the commits in one go, I need to learn how to properly do it.

Copy link
Member

@starkos starkos left a comment

Choose a reason for hiding this comment

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

Looks good, approving. Thanks so much for the contribution!

@starkos starkos merged commit 6ca1219 into premake:master Mar 5, 2020
@redorav
Copy link
Collaborator Author

redorav commented Mar 6, 2020

Thanks for getting it in so quick, I love how premake keeps growing and maturing

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.

2 participants