-
Notifications
You must be signed in to change notification settings - Fork 589
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
generate WIP warnings on the use of WIP messages in C code #240
base: master
Are you sure you want to change the base?
Conversation
ping @amilcarlucas and @hamishwillee |
17f9431
to
071497f
Compare
|
@hamishwillee yes, that's how it would be used on (2), the problem is we run tests with -Werror, so any warnings would make the test fail |
Yes, I want to run the tests, watch them fail, and see the message :-) It was more FMI than anything else, because it isn't obvious (to me) how you could do this if you wanted given that most of the test is autogenerated. If it were me I would additionally add a file level comment about the fact the message is WIP. My concern is users who see a message and try to use it without understanding the implications. You might not consider that a high risk. I'm OK with this fix otherwise. I don't have any magic to merge or approve this. |
When we use similar approach for deprecated tag, wouldn't it be better if that was "default on"? |
yes, I guess so. |
@peterbarker @auturgy Should this be merged? I don't love the implementation in that the whole idea was that WIP stuff should not be used in production. I would have preferred perhaps that the library was built without WIP messages by default and you had to specifically enable them using a build time flag. We are moving away from using WIP in common, but I suspect that dialects may still find it useful to keep WIP, and therefore possibly to have this warning. |
Ideally we’d build with WIP by default in Master/Dev branches but not in production/release branches.
… On 17 Nov 2021, at 4:17 pm, Hamish Willee ***@***.***> wrote:
@peterbarker @auturgy Should this be merged?
I don't love the implementation in that the whole idea was that WIP stuff should not be used in production.
tridge has his reasons, but I doubt that people will think to enable this, which makes it pointless.
I would have preferred perhaps that the library was built without WIP messages by default and you had to specifically enable them using a build time flag.
That of course would be a bit frustrating because it would be all or nothing - you want a WIP thing in ardupilot you have to take the ones in common too.
We are moving away from using WIP in common, but I suspect that dialects may still find it useful to keep WIP, and therefore possibly to have this warning.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
@auturgy Yes, though I guess that would have its own challenges when you cut the release and realize you actually rely on something WIP. Is there any strong feeling on what you want ArduPilot side? |
I also vote to remove WIP items from stable releases. What do you think @tridge? |
A new PR needs to be created because after the #592 merge, the WIP attribute needs to be added to the enumeration. I think that elements with the WIP tag should be ignored by the parser (mavparse.py), not every generator. |
I think so.
I think that's acceptable.
It's kind of neither here nor there. It's not a great look from a standards-perspective having things in |
We need to discuss this in the context of the generated C mavlink headers here: https://github.com/mavlink/c_library_v2 If we were to add an option to That could be a lot of breakage in a short amount of time. This might be considered a transition issue? Should the pre-compiled headers come in two flavours, with-WIP and without? Problem then becomes "what gets built with WIP, what doesn't?". So, for example, do QGC's daily builds ship with WIP messages and the releases not? ArduPilot's "latest" releases with-WIP and "stable" releases without? One big advantage of not including WIP messages is that field definitions can change semantics during development without changing the checksum of the message. Might this lead to dangerous situations? |
@peterbarker FWIW my leaning is not to remove WIP stuff from builds. Had we decided to build without the WIP stuff at the beginning it would have been good/ok. But I think you're right and it might create a lot of breaks now. It would certainly need to be tested and initially default as "not removed". PX4 now does the same thing as ArduPilot and builds the libraries from sources. I don't know what QGC does. But either way, we'd be adverse to changing the generated libraries if we don't know for sure that we won't be breaking others. Longer term the goal is to move all wip out of common, where at all possible: either things are in development.xml or they are actually common, or they are in a dialect. It is slow going. Anyway, my leaning is that this PR now makes sense for dialects. Or we could just close it and move on. Nothing happened for 4 years and we didn't feel that it was a problem :-0 |
As discussed in dev call and mavlink/mavlink#1924 (comment) Our preference is that mavgen builds without WIP entities by default
|
@peterbarker Did you have a chance to discuss this in call? |
#define MAVLINK_WIP __attribute__((warning("MAVLink work in progress"))) | ||
*/ | ||
#ifndef MAVLINK_WIP | ||
#define MAVLINK_WIP | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the default be to print the warning and a custom define can be used to silence it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, but see my question and the response from @tridge above #240 (comment)
Do you think we could make the behaviour as we expect but explicitly disable the warning in tests? We don't particularly want to have warnings in these tests - we want to have them in live environments.
@peterbarker We closed #1924 in December 2022 in favour of this PR added in 2018 on your suggestion. This has basically been blocked on the request to enable the warning by default. I think after 5 years we can agree that the Pymavlink dev team can block doing the "bleedingly obvious correct thing" forever right? How about you update this to fix the merge conflicts and merge as is? That will be very annoying, and mean that that we will continue to have WIP leaking into releases. BUT it does mean that it is possible for developers to more easily get these warnings. Thoughts? |
071497f
to
7000788
Compare
Force-pushed to fix conflicts with has_location changes. |
Thanks very much @peterbarker . What needs to happen to get this in? Do you need more testing, or something else? I can help. |
Shall we wait another 5 years? |
FYI @auturgy - this is the one discussed in the call. |
Just a ping for @peterbarker and @tridge - is there anything we can do to get this in? |
Fix up bug in wip removal Add some test code to see if can debug problem Remove test code added in previous commit Add with_wip to Opts() test class Strip wip elements before parsing Strip out wip command line option Remove unneeded changes
just use: #define MAVLINK_WIP __attribute__((warning("MAVLink work in progress"))) in your code
@hamishwillee we would need to remove the wip tags on the opendroneid messages first, or all builds that use -Werror (including ArduPilot) will fail |
there is also the basic problem that we use -Werror in CI in ArduPilot. So as soon as a new WIP message gets added that we want to use in ArduPilot master (which is quite often) we would fail CI |
we can bring it in default off, just not default on |
@tridge Thank you. So can't you adjust CI to turn this default off in CI - default on everywhere else? If this is not possible then as above, we'd rather get this in default off than have no way to check for WIP.
W.r.t. ODID none of those messages have WIP. There are WIP things though, such as https://mavlink.io/en/messages/common.html#MAV_CMD_DO_ORBIT - I'm happy to do another round of accepting/removing if that is needed. |
we don't want it for developers doing local builds, or in the custom build server. |
OK - thanks. Then let's get it in disabled by default. |
So can we merge this please @tridge / @peterbarker ? |
Ping @tridge |
now reversed it so it generates warnings only if you ask it too
this also makes it more portable