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

Update to uncrustify 0.78.1 #37

Merged
merged 2 commits into from
Apr 4, 2024

Conversation

clalancette
Copy link
Contributor

@clalancette clalancette commented Apr 3, 2024

This matches the version in Ubuntu 24.04.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Contributor Author

CI:

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

@cottsay
Copy link
Contributor

cottsay commented Apr 3, 2024

Isn't this going to break things because of ament/ament_lint#440?

@clalancette
Copy link
Contributor Author

Isn't this going to break things because of ament/ament_lint#440?

It's a good question. I don't think so, because we've done a bunch of work throughout the codebase to make ourselves compatible with 0.78.1 (which is what is in Ubuntu 24.04).

That said, it may be the case that RHEL-9 has a problem because it is an "in-between" version of uncrustify. Here's CI on RHEL to check:

  • RHEL Build Status

@cottsay
Copy link
Contributor

cottsay commented Apr 3, 2024

Here's CI on RHEL to check

I don't think that's going to work without #32

@clalancette
Copy link
Contributor Author

clalancette commented Apr 3, 2024

I don't think that's going to work without #32

Ah, no. It is actually the opposite. It does work (at least locally), because it is taking advantage of the bug that #32 fixes. That is, it fails to detect uncrustify on the system, so it ends up building it from source and using the vendored version of 0.78.1.

The problem is that if we merge in #32, then uncrustify 0.75 is not compatible with 0.78. So we'd have to do another round of changes, and then try to make code style work 3 ways between 0.72, 0.75, and 0.78. I'd really rather not do that.

I wonder if we should do a variant of #32 here. That is, we fix the detection logic, but then we always build from source if the version is not exactly 0.78.1. In that case, we'll start vendoring on Ubuntu 22.04 where we weren't before, but we will otherwise be encoding exactly what we want. What do you think?

@cottsay
Copy link
Contributor

cottsay commented Apr 3, 2024

Ah, no. It is actually the opposite. It does work...

I'd guess I meant that it isn't actually going to test the RHEL system package because it's just going to vendor it with the same version of uncrustify that we've already tested.


So we currently build cleanly with 0.72 and 0.78, if I understand correctly. Should we at least give 0.75 a try before locking the version? If 0.75 doesn't work, should we just bump the requirement to at least 0.78 instead of locking to exactly 0.78?

I've said it before, but the only reason 0.75 isn't getting detected by the vendor package is the result of a combination of the way EPEL built the package and a bug in our code. It's totally possible that other distributions built 0.75 differently and it is successfully detected as satisfying the vendor requirement despite our detection bug.

@clalancette
Copy link
Contributor Author

Should we at least give 0.75 a try before locking the version? If 0.75 doesn't work, should we just bump the requirement to at least 0.78 instead of locking to exactly 0.78?

0.75 doesn't work; I tried locally. While I could spend some time on it and try to make it better, I don't think it is worth my time.

If 0.75 doesn't work, should we just bump the requirement to at least 0.78 instead of locking to exactly 0.78?

I can do that. I suspect that this won't make a difference (every uncrustify version makes changes), but 0.78.1 is the latest released one anyway.

So what I'm going to do is to take the regex fixes from #32 into this one, and then make sure the version is at least 0.78. Then I'll rerun CI.

And make sure we are at least 0.78.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Contributor Author

New CI:

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

@clalancette
Copy link
Contributor Author

The failing test on windows is unrelated to this change, so going ahead and merging this one.

@clalancette clalancette merged commit 17470d5 into rolling Apr 4, 2024
1 check passed
@delete-merged-branch delete-merged-branch bot deleted the clalancette/update-to-uncrustify-0.78.1 branch April 4, 2024 11:33
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