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

Remove env.get logic from defaults #1289

Closed

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Oct 28, 2023

After some local tests, it turns out these never worked in practice. They seemed to work, as the vast majority had simple fallbacks that were able to function identically. However, use_hot_reload tried to use logic from another container entirely; because that value always defaulted to template_debug, the conditional always passed & use_hot_reload was always enabled if no commandline value was specified

The majority of changes in this PR are removing the env.get logic outright, as every other case can substitute the previous fallback value as the default & the end result is functionally identical. use_hot_reload now has a default value of None, meaning it doesn't add any environment option; as such, an explicit assignment is handled before the very first use-case. This was achieved by porting over the expected logic of the previous default value

The only other change was adjusting the compiledb environment check from the env.get function to the dictionary equivalent. This is because I believe we should be using env.get only in scenarios where a value might be null. Otherwise, the fallback value used is redundant, as an explicit bool is guaranteed

EDIT: Fixes #1297

@Repiteo Repiteo requested a review from a team as a code owner October 28, 2023 16:06
@AThousandShips AThousandShips added the topic:buildsystem Related to the buildsystem or CI setup label Oct 28, 2023
@Repiteo Repiteo force-pushed the remove-env-get-from-default branch 4 times, most recently from 454ecd6 to 4b7aa42 Compare November 5, 2023 00:43
@Repiteo
Copy link
Contributor Author

Repiteo commented Nov 5, 2023

A few additions:

  • Implement same revamped env.get logic to is_msvc
  • Slightly change order of disable_exceptions check to ensure that msvc has a chance to be properly evaluated, instead of always defaulting to False

Both serve to fix #1297.

@dsnopek
Copy link
Collaborator

dsnopek commented Nov 6, 2023

Thanks!

Hm. The env.get()s are there in order to support code like:

env = Environment()
env['disable_exceptions'] = False
Export('env')

SConscript("godot-cpp/SConstruct")

So that the author of a GDExtension can change what the defaults are in their case.

With this PR, how would you accomplish the same thing?

Also, this will need a rebase after PR #1299 in order for tests to pass.

• Didn't work in practice, always returning the fallback value instead
@Repiteo
Copy link
Contributor Author

Repiteo commented Nov 6, 2023

That's interesting… I was testing with launch arguments before, but that seems to handle differently than assigning in Python then porting to an exported command. Not sure if the same behavior applies to the main repo, I'll have to test that sometime today Wait so, this all also means that any options in the misc subfiles will need this as well? That feels off

Either way, I suppose this was a bit of a misguided PR. There's still things about it that fix bugs that I want to get committed, but I'll just put those in a separate PR so I can untangle this in its own right

@Repiteo Repiteo marked this pull request as draft November 6, 2023 15:32
@Repiteo
Copy link
Contributor Author

Repiteo commented Nov 9, 2023

Closing this now-draft PR. The core issues have been addressed & some initial assumptions were misinformed. I still want to explore alternatives to the current env.get implementations, but removing them outright without further action isn't the play

@Repiteo Repiteo closed this Nov 9, 2023
@Repiteo Repiteo deleted the remove-env-get-from-default branch March 9, 2024 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archived topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

msvc builds cause warning c4530
4 participants