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 Windows optimized workflow #79

Merged
merged 4 commits into from
May 30, 2024
Merged

Add Windows optimized workflow #79

merged 4 commits into from
May 30, 2024

Conversation

Miepee
Copy link
Contributor

@Miepee Miepee commented May 30, 2024

CI currently has some invalid dependencies, which are these:

	DLL Name: api-ms-win-crt-environment-l1-1-0.dll
	DLL Name: api-ms-win-crt-heap-l1-1-0.dll
	DLL Name: api-ms-win-crt-math-l1-1-0.dll
	DLL Name: api-ms-win-crt-multibyte-l1-1-0.dll
	DLL Name: api-ms-win-crt-private-l1-1-0.dll
	DLL Name: api-ms-win-crt-runtime-l1-1-0.dll
	DLL Name: api-ms-win-crt-stdio-l1-1-0.dll
	DLL Name: api-ms-win-crt-string-l1-1-0.dll
	DLL Name: api-ms-win-crt-time-l1-1-0.dll

I don't know if these are fine to have or not, and what would need to be done to not include them. Currently I just ignore them and only make them fail if a certain env var is set.

You can find the job on my fork here

@Alcaro
Copy link
Owner

Alcaro commented May 30, 2024

That dependency check is intended to stop me from adding dependencies on mingw-specific dlls, like libgcc_s_seh-1.dll. I only want dependencies that are part of a normal Windows installation, so flips.exe is standalone and doesn't need to be shipped with a pile of DLLs.

Those specific dependencies look like you're using a ucrt-targetting mingw. The dependency checker expects msvcrt, since that one's available on older Windows editions, all the way down to XP.

According to https://learn.microsoft.com/en-us/cpp/windows/universal-crt-deployment?view=msvc-170 and https://support.microsoft.com/en-us/topic/update-for-universal-c-runtime-in-windows-c0514201-7fe6-95a3-b0a5-287930f3560c, UCRT is installed on a fully updated Windows 7, and even Vista. I'll count them as being part of a normal Windows installation; I'm fine with adding them to the whitelist. (Simply add a api-ms-win-crt clause to that grep pattern, and delete that now-unnecessary escape hatch.)

Another issue is that that's not properly optimized, you forgot setting the FLAGS variable. You'll need to copy that line from the Linux build script.

And if I download an artifact from your build, I get a zip containing another zip; that's just silly, isn't it?

I could fix the above myself, but I haven't used Windows seriously for ... ten years probably, so you most likely know Windows better than me at this point. I'll need you to double check that I'm not proposing anything silly.

@Miepee
Copy link
Contributor Author

Miepee commented May 30, 2024

And if I download an artifact from your build, I get a zip containing another zip; that's just silly, isn't it?

It is, but this is due to how GH does things.
GH artifacts are always in a STORE'd (read 0-compression) zip. It's a relatively common thing for projects to have that zip-in-a-zip-thing, but since flips is all self-contained, I wouldn't mind changing it here to only have zip->flips. Thought it'd save a bit of bandwidth for whoever is downloading the artifacts.

The proposal to assume that the api-ms-win stuff is part of a default windows installation sounds fine to me.

@Alcaro
Copy link
Owner

Alcaro commented May 30, 2024

The Flips binaries are only about 100-150KB each. I don't think trying to shrink that any further is worth the extra click.

May be worth filing a bug report against github, though. Could be helpful for other projects.

@Miepee
Copy link
Contributor Author

Miepee commented May 30, 2024

May be worth filing a bug report against github, though

Checked the docs, I was wrong.

# The level of compression for Zlib to be applied to the artifact archive.
# The value can range from 0 to 9.
# For large files that are not easily compressed, a value of 0 is recommended for significantly faster uploads.
# Optional. Default is '6'
compression-level:

Copy link
Owner

@Alcaro Alcaro left a comment

Choose a reason for hiding this comment

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

All the more reason to not double-zip it.

Sure, looks good to me. Except it looks like it's failing on your end https://github.com/Miepee/Flips/actions/runs/9302178564/job/25601860598, any idea why? Is the dependency checker doing something it shouldn't?

echo "This script creates a heavily optimized Windows binary. For debugging you're better off using the Makefile directly."

# Set Windows (with gcc) specific optimization flags. These may need to be revisited when the project is build using MVSC.
FLAGS='-Wall -O3 -flto -fuse-linker-plugin -fomit-frame-pointer -fmerge-all-constants -fvisibility=hidden'
Copy link
Owner

Choose a reason for hiding this comment

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

Wonder if -fvisibility=hidden does anything on Windows, it's a pretty Unix-y flag.

But it doesn't error out, so let's keep it.

make-windows.sh Outdated Show resolved Hide resolved
@Miepee
Copy link
Contributor Author

Miepee commented May 30, 2024

any idea why?

because i had echo '...' && exit 1. and a non-zero exit code is treated as a failure.

@Miepee
Copy link
Contributor Author

Miepee commented May 30, 2024

nvm that was not it

@Alcaro
Copy link
Owner

Alcaro commented May 30, 2024

Those flags aren't supposed to fail in Clang, but I haven't tried, and GCC and Clang have a bunch of odd differences.

No, that's not why it's failing. It's failing because the last command in the script is that dependency checker, which returns failure. It's supposed to, but apparently bash exits with the exit code of the last command in the script. Weird, I didn't know that.

Add a line saying true at the end. That'll make that the last command, and will change the exit code to success. And put back the 1 on the exit, so it fails if someone adds a dependency they shouldn't.

@Miepee
Copy link
Contributor Author

Miepee commented May 30, 2024

Windows builds now
https://github.com/Miepee/Flips/actions/runs/9302430719

Decided to use exit 0 to be more explicit instead.

@Alcaro
Copy link
Owner

Alcaro commented May 30, 2024

Sure, looks good to me. Note how this exe is 102KB, now that the optimization flags are in place; the last one was 149KB (though they zip to the same size).

@Alcaro Alcaro merged commit 32c7d02 into Alcaro:master May 30, 2024
6 checks passed
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