Add WinOS console+GUI hybrid back into build/production #779
Conversation
…lt pure GUI build target)
rivy
changed the title
|
Essentially all tests are successful; the only failure was a network timeout failure during preliminary OCaml install/setup. |
.github/workflows/CI.yml
Outdated
| # create and stage text+gui hybrid for Windows | ||
| case '${{ matrix.job.os }}' in windows-*) | ||
| # * clean/remove build artifact(s) | ||
| opam exec -- make -C src clean ## .or.# opam exec -- make clean #.or.# rm "src/${project_exe_stem}${{ steps.vars.outputs.EXE_suffix }}" |
There was a problem hiding this comment.
I think rm "src/${project_exe_stem}${{ steps.vars.outputs.EXE_suffix }}" is plenty sufficient here. Just to avoid the entire rebuild. (The designation as console or GUI application happens at link time, so rebuild is not needed.)
There was a problem hiding this comment.
Yes, since it's only a link step change, I thought that myself. But I wasn't completely sure, so I implemented it as a half measure of rebuild. With just the 'src' clean, the recompilation literally completes within seconds and doesn't change the total build time in any real way, so I left it.
I'll change to the rm ... and re-push if you think its warranted.
There was a problem hiding this comment.
Thinking a bit more about it, perhaps a more relevant improvement is to make this new code a separate job step guarded by an if: runner.os == 'Windows'. I think this makes the build log easier to follow should anything fail. It also makes the scripts simpler (trying to go for a universal build script with GTK on Win is kind of futile at this point, something we have to live with).
|
In general it looks fine to me. Just one comment (see above) but even that is not too important. It would mainly help with GHA build times. |
|
I pushed a change. Let mw know if that's the direction you meant. |
|
This looks good to me (assuming the build completes). Just one remark: make sure not to change whitespace in the later portions of the |
|
I'm watching and am fuzzy on the details, but that's ok -- when you two agree it's ready and CI passes I'll hit merge. |
I've got "format on save" enabled on my editor, so I have to specifically "save without reformatting". And, sometimes, my hardwired I'll reformat the PR commit(s), keeping whitespace otherwise intact. |
|
All tests passing. No extraneous whitespace changes. |
|
You missed one nit comment (#779 (comment)), or maybe not :) Either way, looks good to me. I'm not too sure about the confusion that the users must have with three exe-s. Then again, that confusion is already there with two exe-s, I guess. It could be simpler for first-time (click-starting) users. It's not in scope of this PR, of course. |
rivy
force-pushed
the
x.fix-win-hybrid
branch
2 times, most recently
from
3212340 to
7724c31
Compare
|
I didn't see the note (likely, I was posting from an un-updated page). |
|
Ci hasn't quite passed but I'm going to merge anyway, as the Windows CI infrastructure just seems too flaky. |
Fixes #778.