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

Stop building with -Werror #81

Merged
merged 7 commits into from
Nov 22, 2022
Merged

Conversation

nalimilan
Copy link
Contributor

@nalimilan nalimilan commented Jul 26, 2022

-Werror should not be used in default build flags as it makes the compilation fail every time somebody tries to build with a new version of the compiler which triggers a new warning. It should only be set when running CI.

I saw this when trying to build the RPM for Julia 1.8.0-rc3 on Fedora rawhide (with GCC 12.1.1):

threading.c: In function 'lbt_register_thread_interface':
threading.c:44:5: error: 'strcpy' writing one too many bytes into a region of a size that depends on 'strlen' [-Werror=stringop-overflow=]
   44 |     strcpy(getter_names[idx], getter);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
threading.c:43:34: note: destination object of size [0, 9223372036854775805] allocated by 'malloc'
   43 |     getter_names[idx] = (char *) malloc(strlen(getter));
      |                                  ^~~~~~~~~~~~~~~~~~~~~~
threading.c:46:5: error: 'strcpy' writing one too many bytes into a region of a size that depends on 'strlen' [-Werror=stringop-overflow=]
   46 |     strcpy(setter_names[idx], setter);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
threading.c:45:34: note: destination object of size [0, 9223372036854775805] allocated by 'malloc'
   45 |     setter_names[idx] = (char *) malloc(strlen(setter));
      |                                  ^~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:37: build/threading.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make[1]: Leaving directory '/builddir/build/BUILD/julia-1.8.0-beta3/deps/scratch/blastrampoline-d32042273719672c6669f6442a0be5605d434b70/src'
make: *** [/builddir/build/BUILD/julia-1.8.0-beta3/deps/blastrampoline.mk:14: scratch/blastrampoline-d32042273719672c6669f6442a0be5605d434b70/build-compiled] Error 2
make: Leaving directory '/builddir/build/BUILD/julia-1.8.0-beta3/deps'

The warning probably deserves some attention but the problem isn't new I guess so there's no reason to worry the particular user who happened to build with a new compiler version.

`-Werror` should not be used in default build flags as it makes the compilation fail every time
somebody tries to build with a new version of the compiler which triggers a new warning.
It should only be set when running CI.
@ViralBShah
Copy link
Collaborator

Merge?

@ViralBShah ViralBShah closed this Aug 8, 2022
@ViralBShah ViralBShah reopened this Aug 8, 2022
@staticfloat
Copy link
Member

We should add this back on in CI as a part of this PR then.

@nalimilan
Copy link
Contributor Author

I've added a commit which seems like it should have done that, but the logs show that the compiler was called without -Werror. Any ideas?

@giordano
Copy link
Collaborator

I think flags are overridden at

cflags_add = needs_m32() ? "-m32" : ""
dir = mktempdir()
srcdir = joinpath(dirname(@__DIR__), "src")
run(`$(make) -sC $(pathesc(srcdir)) CFLAGS=$(cflags_add) ARCH=$(Sys.ARCH) clean`)
run(`$(make) -sC $(pathesc(srcdir)) CFLAGS=$(cflags_add) ARCH=$(Sys.ARCH) install builddir=$(pathesc(dir))/build prefix=$(pathesc(dir))/output`)

@nalimilan
Copy link
Contributor Author

Thanks, seems to work now!

Copy link
Collaborator

@giordano giordano left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I think the setting of cflags can be simplified in tests

test/utils.jl Outdated Show resolved Hide resolved
@nalimilan
Copy link
Contributor Author

Should be good now.

@giordano giordano merged commit 95aef37 into JuliaLinearAlgebra:main Nov 22, 2022
@giordano
Copy link
Collaborator

Side note, I think the warning was fixed by #71, right?

@staticfloat
Copy link
Member

Thanks, @nalimilan!

@nalimilan nalimilan deleted the patch-1 branch December 6, 2022 08:55
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.

4 participants