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 build options configuration for Visual Studio projects #79238

Merged

Conversation

CncealDVRflectN
Copy link
Contributor

Fixes #79127

@CncealDVRflectN CncealDVRflectN requested a review from a team as a code owner July 9, 2023 11:22
@Calinou Calinou added bug platform:windows topic:buildsystem cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Jul 9, 2023
@Calinou Calinou added this to the 4.x milestone Jul 9, 2023
@anvilfolk
Copy link
Contributor

This needs a rebase!

Also, I think special care needs to be had when using scu_build, though that is perhaps another problem for another PR, since that changes the files themselves as far as I can tell, and I had trouble with files not being in the solution when I used scu_build alongside vsproj. Perhaps it's easier to say in the documentation that those options aren't compatible?

@anvilfolk
Copy link
Contributor

I recently dug into this while looking into #81144, and that made me look into build options more carefully, and I'm not sure that we fully need all the options to be passed in. For example, fast_unsafe is already included by dev_build, and tests by dev_mode.

I also wonder whether there aren't options that really shouldn't be passed in 🤔

Are there any other options that we would really want passed in? Maybe things like disabling certain modules, or is that already in?

@CncealDVRflectN
Copy link
Contributor Author

I don't have a lot of knowledge of Godot and its build system, so I don't know about any specific flags that should be ignored specifically in Visual Studio projects, except those that I've filtered out in this PR. As I understand, Visual Studio just triggers SCons on build. If so, than I think we should pass as much user input as possible to VS projects, so Visual Studio Godot builds would be the same if we built them from terminal.

Of course, there are exceptions. For instance, the vsproj flag itself. I've filtered it out in this PR as it's undesirable to regenerate a VS project on every build.

In the end, I think it's better to exclude invalid options from user input for VS projects, than to just manually add a check for every new supported option to this VS specific script. This approach will make every new option available for VS projects by default, which is useful since there are a lot of contributors who don't use Visual Studio and who probably don't know that they should be adding their new options to this VS specific method. And as a result, it'll minimize the number of situations when contributors will get different builds using VS project compared to builds from terminal.

Comment on lines +784 to +785
# The "platform" option is ignored because only the Windows platform is currently supported for VS projects
filtered_args.pop("platform", None)
Copy link
Member

Choose a reason for hiding this comment

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

This is fine for now, but we might want to look into supporting generating a VS project for other platforms.

Notably we should have a GDK port for Windows eventually, and console ports also typically require using Visual Studio.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks great! I was just ranting about why nobody was doing something like this, and why the vsproj option was still so limited with a list of hardcoded options being passed.

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Sep 7, 2023
@akien-mga
Copy link
Member

I recently dug into this while looking into #81144, and that made me look into build options more carefully, and I'm not sure that we fully need all the options to be passed in. For example, fast_unsafe is already included by dev_build, and tests by dev_mode.

The vsproj config shouldn't need to do more logic than SCons itself does, since all that happens is just that VS calls scons when you press "Build" (the only added value is generating different target presets, and the boilerplate needed for VS to handle the source code as a project/solution).

So while specifying scons dev_build=yes fast_unsafe=yes is redundant, SCons already handles that. The latter will actually be used to override what dev_build would default to - so you can do scons dev_build=yes fast_unsafe=no and it will not use the fast_unsafe path. So the VS config should work the same and shouldn't filter out either option.

@anvilfolk
Copy link
Contributor

Makes a lot of sense!

Just for future documentation purposes, scu_build is still broken with vsproj, since the VS solution includes the files generated by the SCUs and not the original ones IIRC, which leads to some really funky attempts at compilation. I believe the solution there will be to generate the vsproj before creating SCUs? TIWAGOS since I don't really understand SCUs 😅

@akien-mga akien-mga merged commit 97f3f97 into godotengine:master Sep 7, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 20, 2023
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.

Inconsistent build options in a generated Visual Studio project
5 participants