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

gnome.generate_gir(): add 'fatal_warnings' argument #7130

Closed
gdesmott opened this issue May 13, 2020 · 12 comments · Fixed by #7282
Closed

gnome.generate_gir(): add 'fatal_warnings' argument #7130

gdesmott opened this issue May 13, 2020 · 12 comments · Fixed by #7282
Labels

Comments

@gdesmott
Copy link

When building with werror=enabled gnome.generate_gir() should pass --warn-error to the gir generator so its own warnings are fatal as well.

@tp-m
Copy link
Member

tp-m commented May 13, 2020

I'm very wary of this, not sure this is a good idea at all to link this to the global meson --werror option.

It's pretty difficult in a complex code base to make g-ir-scanner warning-free even more so whilst supporting a range of gobject-introspection versions.

Maybe we should look into adding either module options, or a new kwarg (which projects could then link to a custom option)?

@gdesmott
Copy link
Author

We can already do it manually by passing --warn-error to the extra_args kwarg. I was considering how to do that in GStreamer when I figured it may be useful to have it directly into meson.

Maybe this could be gated with a specific warning-level?

@tp-m
Copy link
Member

tp-m commented May 13, 2020

Maybe this could be gated with a specific warning-level?

Right, but my concern was basically the ability to do the normal -Wall -Werror for C/C++ compilers.

Maybe one could have something like --werror-domains=c,cpp,introspection?

Or maybe I'm just worried unnecessarily, but I just don't feel g-i has the same kind of maturity level as a proper compiler and we'll need to be able to work around wrong warnings or it otherwise getting confused (e.g. it's pure luck that your trick in gst-plugins-base worked and g-i didn't pick up on the types being different, isn't it? What do we do if it starts warning about that in the next version?)

@gdesmott
Copy link
Author

Can't users just workaround warnings by annotating the symbol with (skip)? If most errors lead to the gir to be generated with introspectable=0 it doesn't change anything for gir users and will force developers to acknowledge those problems.

@tp-m
Copy link
Member

tp-m commented May 13, 2020

Maybe, I don't know if all/most warnings lead to introspectable=0. If yes then that seems reasonable to me.

@gdesmott
Copy link
Author

Looks like silenting those warnings is actually not that easy. So I agree with you, using werror is not a good idea in the current gir state.

What about adding a fatal_warning kwars to gnome.generate_gir() which would accept either a boolean or a feature option? So projects could just do something like this:

gnome.generate_gir(...
  fatal_warning : get_option('gir-fatal-warnings')
)

@tp-m
Copy link
Member

tp-m commented May 14, 2020

What about adding a fatal_warning kwars to gnome.generate_gir() which would accept either a boolean or a feature option? So projects could just do something like this:

I like that.

@tp-m
Copy link
Member

tp-m commented May 14, 2020

adding a fatal_warning kwarg

Maybe plural though - fatal_warnings? That matches use elsewhere I think (e.g. G_DEBUG=fatal_warnings)

@gdesmott gdesmott changed the title gnome.generate_gir() should enable fatal warnings if werror is enabled gnome.generate_gir(): add 'fatal_warnings' argument May 14, 2020
@dcbaker dcbaker added the gnome label May 15, 2020
xclaesse added a commit to xclaesse/meson that referenced this issue Jun 9, 2020
@xclaesse
Copy link
Member

xclaesse commented Jun 9, 2020

What about adding a fatal_warning kwars to gnome.generate_gir() which would accept either a boolean or a feature option? So projects could just do something like this:

Why would it be a feature option? "auto" would make no sense here... If it's a feature option in your project for whatever reason, you can just pass get_option('gir-fatal-warnings').enabled(), or not get_option('gir-fatal-warnings').disabled(), depending on the meaning you want to give to auto.

Maybe plural though - fatal_warnings? That matches use elsewhere I think (e.g. G_DEBUG=fatal_warnings)

+1, we have meson setup --fatal-meson-warnings already, which itself is inspired from glib.

@gdesmott
Copy link
Author

Why would it be a feature option? "auto" would make no sense here... If it's a feature option in your project for whatever reason, you can just pass get_option('gir-fatal-warnings').enabled(), or not get_option('gir-fatal-warnings').disabled(), depending on the meaning you want to give to auto.

Can we do fatal_warning : get_option('gir-fatal-warnings') with a boolean option? Then I think it's fine then.
As long as we can enable/disable it easily I'm happy. :)

@xclaesse
Copy link
Member

Yes that works

@tp-m
Copy link
Member

tp-m commented Jun 10, 2020

Why would it be a feature option? "auto" would make no sense here.

auto would simply mean "I don't care one way or another and will leave it up to circumstances", it provides space for doing different things by default later.

Not very important in this case though.

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

Successfully merging a pull request may close this issue.

4 participants