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

[DRAFT] example of setting -Werror in autotools ./configure CI script #803

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

RossBencina
Copy link
Collaborator

Either include in #800 or follow up from 800

I don't intend to use both options. Choose one option, either

  1. configure step, OR
  2. make step

See discussion at https://stackoverflow.com/questions/32792692/cflags-in-configure-script

@RossBencina RossBencina added the CI Continuous Integration including GitHub Actions label Mar 28, 2023
@RossBencina RossBencina marked this pull request as draft March 28, 2023 00:22
@RossBencina
Copy link
Collaborator Author

Right now I don't have an environment to test this. Seems to be failing CI. @dechamps if feasible could you check whether either of these options work for re-enabling -Werror?

@dechamps
Copy link
Contributor

@dechamps if feasible could you check whether either of these options work for re-enabling -Werror?

It works if you remove the change to make and only pass CFLAGS=-Werror to configure. I'm not too familiar with autoconf, but I suspect the reason is because if you do make CFLAGS=… then you are overriding CFLAGS that are generated by configure, including critical options like include paths (hence the portaudio.h: No such file or directory error).

Of course, once you fix that, you will then also need to reproduce this allowlist for the build to succeed:

portaudio/CMakeLists.txt

Lines 36 to 39 in 65df2c4

-Wno-error=deprecated-declarations # https://github.com/PortAudio/portaudio/issues/213 https://github.com/PortAudio/portaudio/issues/641
-Wno-error=incompatible-pointer-types
-Wno-error=int-conversion
-Wno-error=stringop-overflow

Which then begs the question of whether we really want to have this list be in two places at the same time and having to keep both in sync. (This is where I would typically add "this is why it's a bad idea to try to maintain multiple build systems" but we both know how this discussion will end, so let's not go there.)

@RossBencina
Copy link
Collaborator Author

Thanks for checking. I've reverted to setting environment variables rather than passing parameters to ./configure. In both cases the CI passed. Strange.

Which then begs the question of whether we really want to have this list be in two places at the same time and having to keep both in sync.

To be honest, I'd prefer to not have such a list at all. -Wno-error=deprecated-declarations is probably unavoidable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration including GitHub Actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants