Alcaro / Flips

Floating IPS is a patcher for IPS and BPS files.
Other
334 stars 46 forks source link

Add Windows optimized workflow #79

Closed Miepee closed 4 months ago

Miepee commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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:
Miepee commented 4 months ago

any idea why?

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

Miepee commented 4 months ago

nvm that was not it

Alcaro commented 4 months ago

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 commented 4 months ago

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

Decided to use exit 0 to be more explicit instead.

Alcaro commented 4 months ago

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).