Skip to content

Conversation

@topolarity
Copy link
Contributor

See JuliaLang/julia#54836 (comment)

I'm not sure we're ready to enable -Werror comprehensively on Windows (esp. retroactively), but this warning is a common one that is almost aways a bug to encounter.

@giordano
Copy link
Member

I'm not sure we're ready to enable -Werror comprehensively on Windows

Last time I looked at a Windows build log there were still loads of warnings, but that was long time ago probably. If now they're all gone I think it should be safe to enable -Werror in the pipeline for master: we should have separate pipelines for the backport branches, right? And if a PR fails because it's on top of a very old revision of master than e rebase/merge should be enough to fix that.

@topolarity
Copy link
Contributor Author

I think we still have a few warnings unfortunately, such as:

C:/msys64/mingw32/i686-w64-mingw32/include/psdk_inc/intrin-impl.h:2164:1: warning: array subscript 0 is outside array bounds of 'long unsigned int[0]' [-Warray-bounds]
--
  | 2164 \| __buildreadseg(__readfsdword, unsigned __LONG32, "fs", "l")
  | \| ^~~~~~~~~~~~~~

@topolarity
Copy link
Contributor Author

topolarity commented Sep 22, 2025

On second thought... we should be able to get away with

-Werror -Wno-error=undef -Wno-error=unused-but-set-variable -Wno-error=strict-prototypes -Wno-error=attributes -Wno-error=array-bounds -Wno-error=infinite-recursion -Wno-error=c++-compat

Several of those seem straightforward to resolve, but it'd be nice to get the -Werror coverage in the mean time for those that can't.

Do you know how to provide a string with spaces to the JL_CFLAGS in the custom .arches format?

@giordano
Copy link
Member

Do you know how to provide a string with spaces to the JL_CFLAGS in the custom .arches format?

No ☹️ I presume you already considered playing with various quotes?

@topolarity topolarity force-pushed the ct/windows-Werror branch 3 times, most recently from c891063 to 3dfc824 Compare September 23, 2025 00:29
@topolarity
Copy link
Contributor Author

topolarity commented Sep 23, 2025

I think I found an incantation that works

(ShellCheck is not in love with it, but I can find no simple alternative that works and the wiki does specifically allow exceptions in https://www.shellcheck.net/wiki/SC2207)

Looks like this needs treehash signing before CI will give it a test run.

@topolarity topolarity changed the title Use -Werror=implicit-function-declaration on Windows builds Use -Werror (with some exceptions) on Windows builds Sep 23, 2025
@topolarity topolarity changed the title Use -Werror (with some exceptions) on Windows builds Use -Werror (with exceptions) on Windows builds Sep 23, 2025
@topolarity topolarity force-pushed the ct/windows-Werror branch 4 times, most recently from 7f4c67f to 0fbcf93 Compare September 23, 2025 01:57
fingolfin pushed a commit to JuliaLang/julia that referenced this pull request Sep 24, 2025
Should fix:
  - `[-Wstrict-prototypes]`
  - `[-Wattributes]`
  - `[-Wc++-compat]`
  - `[-Wunused-but-set-variable]`
  - `[-Wdiscarded-qualifiers]`
  - `[-Wundef]`

This leaves:
  - `[-Warray-bounds]`
  - `[-Winfinite-recursion]`

see JuliaCI/julia-buildkite#484
We're not ready to enable `-Werror` comprehensively on Windows, but
this will include most warnings and prevent us from accidentally
adding new failures.
@IanButterworth IanButterworth merged commit 32e5db8 into JuliaCI:main Sep 24, 2025
4 of 7 checks passed
@ararslan
Copy link
Member

We can't build v1.12.0-rc3 binaries for Windows because of this:

On x86-64 (log):

C:/workdir/src/debuginfo.cpp:307:14: error: variable 'catchjmp' set but not used [-Werror=unused-but-set-variable]
  307 |     uint8_t *catchjmp = NULL;
      |              ^~~~~~~~

On i686 (log):

C:/workdir/src/module.c:16:30: error: redeclaration of 'jl_bpart_get_kind' with different visibility (old visibility preserved) [-Werror]
   16 | EXTERN_INLINE_DEFINE uint8_t jl_bpart_get_kind(jl_binding_partition_t *bpart) JL_NOTSAFEPOINT;
      |                              ^~~~~~~~~~~~~~~~~

The commit that fixed the warning on x86-64 (JuliaLang/julia#59634) isn't on the release branch. That commit wouldn't fix the warning on i686 though.

I think we need to revert this in order to build the release binaries for Windows, then reapply (some version of?) it, and in the meantime backport the aforementioned commit to all relevant branches so that the next release won't hit the same situation.

cc @KristofferC

@giordano
Copy link
Member

Don't the various minor versionz have separate pipelines?

@DilumAluthge
Copy link
Member

Don't the various minor versionz have separate pipelines?

Yes, but by default each pipeline uses julia-buildkite#master.

@DilumAluthge
Copy link
Member

It's easy to point a pipeline to a different branch. E.g. we could create a julia-buildkite#release-1.12 branch, and then point the 1.12 Buildkite config to that branch.

But by default, it defaults to julia-buildkite#master.

@DilumAluthge
Copy link
Member

Let's proceed with the revert for now, just to unblock Alex.

This weekend, I'll set up some release branches in this repo.

And then we can re-land this PR once I have the release branches set up.

@ararslan
Copy link
Member

Looks like we have a few release branches on this repo, but the most recent is for 1.9.

@DilumAluthge
Copy link
Member

Looks like we have a few release branches on this repo, but the most recent is for 1.9.

I'll make release branches for 1.10, 1.11, and 1.12 then.

kodiakhq bot pushed a commit that referenced this pull request Sep 27, 2025
Reverts #484 per
#484 (comment).

We can't build 1.12.0-rc3 binaries for Windows due to `-Werror`.
xal-0 pushed a commit to xal-0/julia that referenced this pull request Sep 30, 2025
Should fix:
  - `[-Wstrict-prototypes]`
  - `[-Wattributes]`
  - `[-Wc++-compat]`
  - `[-Wunused-but-set-variable]`
  - `[-Wdiscarded-qualifiers]`
  - `[-Wundef]`

This leaves:
  - `[-Warray-bounds]`
  - `[-Winfinite-recursion]`

see JuliaCI/julia-buildkite#484
@DilumAluthge
Copy link
Member

Looks like we have a few release branches on this repo, but the most recent is for 1.9.

I'll make release branches for 1.10, 1.11, and 1.12 then.

The release branches exist now.

kodiakhq bot pushed a commit that referenced this pull request Nov 4, 2025
Re-land #484 (as
requested in
#487 (comment))

This will need the treehashes to be signed again.

---------

Co-authored-by: Ian Butterworth <[email protected]>
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.

5 participants