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

Fix VS project creation #66718

Merged
merged 1 commit into from
Oct 4, 2022
Merged

Fix VS project creation #66718

merged 1 commit into from
Oct 4, 2022

Conversation

afestini
Copy link

@afestini afestini commented Oct 1, 2022

Fixes #66664.

The introduction of a dev option created a mismatch between number of build targets and variants, causing the VS project creation to fail.

PR creates the missing variants for dev/non-dev.

@akien-mga
Copy link
Member

Our python formatter (black with line length configured to 120) wants the long line wrapped: https://github.com/godotengine/godot/actions/runs/3165660414/jobs/5155026483

I'd suggest moving the length calculation to a local variable to keep it to 2 lines instead of what the formatter wants to do here.

Please edit the commit with git commit --amend when fixing it.

@afestini
Copy link
Author

afestini commented Oct 2, 2022

Done. Glad I'm not the only one that prefers to avoid line breaks (even more so in Python).
I was going for "variant_count", to make it clearer, but the indentation level only allowed for "count". Given how short the function is, it should still be clear enough.

@TechnoPorg
Copy link
Contributor

Maybe vsproj=yes should be added to the GitHub Windows builds, to prevent future regressions of this nature?

@afestini
Copy link
Author

afestini commented Oct 3, 2022

Changed the logic after running into a problem with binary output names. Names were created for dev_build=yes and =no, but since variant names were identical and collapsed to only 3, the binary would always be named .dev, even for non-dev builds.

The new version only creates the names needed depending on whether dev_build is set or not, rather than creating duplicate variants that get filtered out by VS. It seems like the NMake command line is also only based on the env variable.

A cleaner fix could involve:
-create NMake command line individually for each variant (right now it's just duplicated, since it was generic enough to work for all combinations until dev_build entered the picture)

-make "dev" part of the variant names, increasing the number from 3 to 6 configurations (if it doesn't create any issues in other parts).

@akien-mga
Copy link
Member

That seems fine if that's the expected workflow for VS users. I never use it so I don't know which is preferred.

My intention was that users would be able to select whether they want to build and run a dev build from a drop-down list, like they can choose if they want to build the editor, release template or debug template, i.e.:

  • Editor
  • Editor (dev build)
  • Debug template
  • Debug template (dev build)
  • Release template
  • Release template (dev build)

And thus, like target, or tools before the refactoring, the value of dev_build passed together with vsproj wouldn't matter.

But I'm also fine with the current solution for the time being, we can reassess if users do want something like what I list above.

@akien-mga akien-mga merged commit 9a67c3d into godotengine:master Oct 4, 2022
@akien-mga
Copy link
Member

akien-mga commented Oct 4, 2022

Thanks! And congrats for your first merged Godot contribution 🎉

@afestini
Copy link
Author

afestini commented Oct 4, 2022

Those six options were my first attempt, but as Eric pointed out, that is causing issues. There seem to be some unexpected dependencies on the names. I also have to correct myself on duplicating the command line after trying to figure out how to create different ones. I only see the one and cmdargs seems to be the way to pass different additional options to scons.

So I agree that this is more of a hotfix. If handling all 6 configs can be done with relatively small changes, I might open another PR, but I'm hesitant to mess around too much without a deeper understanding how the whole build system works together (especially across platforms I don't have and can't test).

@afestini afestini deleted the fix_vs_project_creation branch October 4, 2022 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Visual Studio vsproj generation broken after #66242
4 participants