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 win scons #359

Merged
merged 46 commits into from
Jun 12, 2024
Merged

Fix win scons #359

merged 46 commits into from
Jun 12, 2024

Conversation

NQNStudios
Copy link
Collaborator

There's some messiness in this PR with trying things, reverting them, etc. It should probably all be squashed, but I can now make a build with scons on Windows that actually launches.

SConstruct Outdated
elif platform == "win32" and subprocess.call(['where', '/Q', 'makensis']) == 0:
env.VariantDir("#build/pkg", "pkg/win")
SConscript("build/pkg/SConscript")
#elif platform == "win32" and subprocess.call(['where', '/Q', 'makensis']) == 0:
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be uncommented or otherwise fixed before I can merge it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The installer part is having weird problems. I didn't want to deal with it because I think an auto-updating deployment target like Itch or Steam is preferable to having an installer at all.

Copy link
Member

@CelticMinstrel CelticMinstrel Jun 11, 2024

Choose a reason for hiding this comment

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

Putting it on Itch or Steam is fine, but we should support a standalone installer build as well for the benefit of people who don't like using services like Itch or Steam. What kind of weird problems was it having? Maybe there's an issue of using deprecated NSIS stuff or something…

FLAC
bz2
brotlidec
brotlicommon
Copy link
Member

Choose a reason for hiding this comment

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

Something seems to be wrong here. What on earth is "brotli", and why is bz2 being bundled when we don't use it?

I don't think we use vorbis/ogg/FLAC, either, though it wouldn't hurt to include it for use by mods someday. Including freetype is fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree it seems wrong, but the Visual Studio build outputs all of those DLLs, and the scons build won't launch without them bundled.

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean it "outputs" all of those DLLs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They're all in the build directory Visual Studio outputs, and if you rename one of them to something else, the binary won't launch.

Copy link
Member

Choose a reason for hiding this comment

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

So then the question is, what exactly is causing them to be required? They shouldn't be required, since we don't use them anywhere. Are they transitive dependencies of something else? If so, the something else should be built in a way that doesn't require them.

@NQNStudios
Copy link
Collaborator Author

I can't get the 32-bit build to work locally despite quadruple-checking all the lib paths. The compiler isn't finding Zlib, but I'm 100% sure I have an x86 build of zlib installed where scons is checking for it. This is frustrating.

I want to suggest separating out some concerns.

Without the PR, we have these problems:

  • impossible to make a 32-bit build with scons on windows
  • impossible to make a 64-bit build with scons on windows
  • no installer generated by any Windows build
  • windows scons logic missing so many required steps, I struggle to imagine how it ever worked, and/or what manual steps were required locally to make it work, but aren't documented anywhere
  • unwanted DLLs required by the windows builds

With the PR, the situation becomes:

  • impossible to make a 64-bit build with scons on windows
  • windows scons logic missing so many required steps, I struggle to imagine how it ever worked, and/or what manual steps were required locally to make it work, but aren't documented anywhere
  • impossible to make a 32-bit build with scons on windows
  • no installer generated by any Windows build
  • unwanted DLLs required by the windows builds

I would suggest that we do this:

  1. Open an issue for the win-scons 32-bit build
  2. Open an issue for the win-scons installer generation
  3. Open an issue for the unwanted DLLs
  4. Set the default win-scons build to 64-bit, so contributors following the README should get a successful result by default
  5. Merge the PR

@CelticMinstrel
Copy link
Member

I struggle to imagine how it ever worked

I'm not certain that it ever worked. When I built on Windows, I (almost?) always used MSVC 2013, not scons.

I agree on points 1, 3, and 4. For point 2, I would like the installer generation uncommented, but it can be set to run only if explicitly requested (so, running scons won't try to generate an installer, but running something like scons package will).

@NQNStudios
Copy link
Collaborator Author

I did all the things.

@CelticMinstrel
Copy link
Member

Is it okay to squash all of this into a single commit? And if so, do you want to write up a summarized commit message?

@NQNStudios
Copy link
Collaborator Author

I'm pretty comfortable squashing all of this one without doing a summary. 🤷‍♀️

@CelticMinstrel CelticMinstrel merged commit 4a7d145 into calref:master Jun 12, 2024
6 checks passed
@NQNStudios NQNStudios deleted the fix-win-scons branch September 8, 2024 20:32
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