search

ament / uncrustify_vendor

CMake shim over the uncrustify library: https://github.com/uncrustify/uncrustify
Apache License 2.0
0 stars 9 forks source link

Update to uncrustify 0.78.1 #37

Closed clalancette closed 7 months ago

clalancette commented 7 months ago

This matches the version in Ubuntu 24.04.

clalancette commented 7 months ago

CI:

cottsay commented 7 months ago

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

clalancette commented 7 months ago

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:

cottsay commented 7 months ago

Here's CI on RHEL to check

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

clalancette commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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.

clalancette commented 7 months ago

New CI:

clalancette commented 7 months ago

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