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

Vendor uncrustify when system has >= 0.75 #33

Closed
wants to merge 1 commit into from

Conversation

cottsay
Copy link
Contributor

@cottsay cottsay commented Mar 28, 2023

This version of uncrustify introduces changes to the configuration file which are not backwards compatible with older configuraitons, causing all invocations of ament_uncrustify to fail.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

This version of uncrustify introduces changes to the configuration file
which are not backwards compatible with older configuraitons, causing
all invocations of ament_uncrustify to fail.

Signed-off-by: Scott K Logan <[email protected]>
@cottsay cottsay self-assigned this Mar 28, 2023
@clalancette
Copy link
Contributor

I've got a couple of different thoughts about this.

First, I was wondering why we needed this at all, given that RHEL-9 has uncrustify 0.75 and seems to be green on the buildfarm. But looking closer, it looks like we are unconditionally vendoring uncrustify 0.72 on RHEL-9 through a mechanism I don't understand. Should we look into this and fix it?

Second, this is kicking the can down the road. We are probably going to have to deal with this problem for Ubuntu 24.04 anyway. So should we have ament_uncrustify attempt to detect the version and use the appropriate configuration file instead?

@cottsay
Copy link
Contributor Author

cottsay commented Mar 29, 2023

Should we look into this and fix it?

Already addressed in #32.

Second, this is kicking the can down the road.

My aim is to get the bug in #32 fixed without regressing RHEL 9, and this is the lowest effort path to do that. Forking the config might be a reasonable thing to do - maybe we should engage the ament_lint maintainers about that.

@clalancette
Copy link
Contributor

My aim is to get the bug in #32 fixed without regressing RHEL 9, and this is the lowest effort path to do that. Forking the config might be a reasonable thing to do - maybe we should engage the ament_lint maintainers about that.

Yeah, I'll suggest we attempt to do this. We still have time before the freeze, and I think it would fall under the "bug fix" category anyway.

@cottsay
Copy link
Contributor Author

cottsay commented Mar 29, 2023

Yeah, I'll suggest we attempt to do this. We still have time before the freeze, and I think it would fall under the "bug fix" category anyway.

So should we wait to merge #32 until ament_cppcheck can handle it, or regress the cppcheck test results in RHEL 9 for the time being?

@clalancette
Copy link
Contributor

So should we wait to merge #32 until ament_cppcheck can handle it, or regress the cppcheck test results in RHEL 9 for the time being?

I'd prefer if we keep things green, so I'd say let's hold off on that one until we have the real solution in place.

@cottsay
Copy link
Contributor Author

cottsay commented May 26, 2023

Tracking this in ament_lint as ament/ament_lint#440.

@cottsay cottsay closed this May 26, 2023
@cottsay cottsay deleted the cottsay/max_version branch May 26, 2023 21:16
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.

2 participants