Protobuf 3.11 for ola0.10 #1630
Conversation
|
@peternewman: I had selected the wrong base branch at first, but corrected that later. However, Travis is not re-building it. Is there any way I can re-trigger the build or is a permission on Travis required that I don't have? |
The later commit has done the trick. I think I've had the same problem in the past and only a fresh commit seems to get it in sync from memory. |
peternewman
left a comment
peternewman
left a comment
There was a problem hiding this comment.
A few minor comments.
You should also stick your name in the AUTHORS bit and add this to NEWS.
| env: | ||
| - TASK='compile' | ||
| - CPPUNIT='1.14' | ||
| - PROTOBUF='latest' |
There was a problem hiding this comment.
We should expand the matrix a bit (probably to match master), but I can do that if you'd rather?
There was a problem hiding this comment.
Parking that for a later PR.
Yep, true. I also thought the same that a new commit will trigger Travis. Good thing I found another thing that needs changing ;)
Will do, good remark. In this PR (It'd be in 0.10 only then) or a new set of PRs? About the travis matrix: Yes, I can do so. While merging I tried to be as less-invasive as possible. I diffed this .travis.yml against the one from master and added two more macOS runs. Now it should build against Protobuf 3.2 and 'latest'. However, in the 0.10-branch, those are under |
We (well I), pull 0.10 into master fairly regularly, hence why I generally suggest targetting 0.10 if it's a bugfix, but master for new features, so you can just add those bits to 0.10 and they'll percolate through.
If those OS X builds work (or at least only fail due to flaky tests), they can be in full, not just in |
|
@peternewman you're right, the current travis file no longer work, because according to https://formulae.brew.sh/formula/protobuf, the oldest version of protobuf that can be easily installed on macOS using homebrew is v3.6. So I would propose to come back to your proposal that I don't touch the travis files in this PR and this can be tackled after this PR has been merged. However, the build failures on macOS (see Travis logs) seem pretty consistent. Might be worth another look. However, on Ubuntu it all look good. So I don't think that these changes broke the build. |
Yep fair enough, we can do the other bit in parallel.
Great thanks.
Agreed, the current Mac failures look like real ones not race-conditions. We need to get them fixed ideally. |
Deleting the caches (i.e. effectively doing a make clean) hasn't fixed the OS X issues unfortunately. |
|
@peternewman : Are you still able to build 0.10.7 (release, without any patches, against an older version of protobuf)? I'd be interested in the content of the file I am able to reproduce the build error on my Linux machine. I also have something that compiles successfully. |
Not quite what you asked, but OLA 0.10 branch (i.e. what you're merging into) still builds fine against Protobuf 2.6.1 on Ubuntu 16.04: No real surprise there as OLA's Travis CI is still using Xenial (until #1584 goes in). I could try the release if you want, but I'm not sure what difference it makes.
Especially the function
I just build in the src directory:
We don't need the two OLAs to be inter-compatible, as long as the patch doesn't break an older release. Do you want to push the patch somewhere else and I can try compiling it? |
… we fix OpenLightingProject#1192, rather than a confusing compiler error" This reverts commit d999217.
…lds in allow_failures section. According to https://formulae.brew.sh/formula/protobuf, [email protected] is the oldest version available.
kripton
force-pushed
the
protobuf-3.11-for-ola0.10
branch
from
04dafab to
3f72d4a
Compare
|
Phew, that was a long list, I didn't expect that :) I've also cleaned up the git history of this branch and updated the For Travis, I kept my fingers off for now, might come in a later PR. Yes, mostly related to #1584. Any reason why you are using Let's see if Travis builds this PR stable now. If you need help merging the protobuf fixes from 0.10 into master, I'd be glad to help. |
|
Travis tasks with non-latest protobuf (= [email protected]) are failing on macOS. Linux is fine. |
|
Side note: I've now configured Travis to work on my fork of ola. So I can experiment with it without making a PR here |
kripton
force-pushed
the
protobuf-3.11-for-ola0.10
branch
from
b424f3c to
3f72d4a
Compare
|
@peternewman : Could you test the current state of the PR on your environments? Travis is failing for iOS for the older Protobuf versions and I currently feel unable to fix this myself :/ |
Make and make check ran fine for me. I've not done anything more intensive yet. Having had a quick look, it seem Travis OS X is failing to install the older Protobuf properly via Homebrew, rather than your code breaking things. |
I've just realised, looking back, we had the same behaviour on master with Travis too: |
|
All done. Tested it with protobuf 3.11.4. Make test looks good on Linux amd64: plus manually tested with Still unsure about the travis file, even when merging latest |
|
According to Travis: Linux builds and test fine, macOS with gcc is also fine, with clang one flaky test failed, all other tests are good 👍 |
|
I've copied the .travis.yml from branch |
That's great news!
No, you should leave it as is, you've fixed it! If you look at the previous run on 0.10 ( https://travis-ci.org/github/OpenLightingProject/ola/builds/680355971 ) it wasn't even doing any Mac runs successfully. At least it's now doing one! For some reason the trick I used to use on the pinned version (now 3.6) isn't working anymore, but one of us can fix that in a later PR, or as part of #1584 . |
peternewman
left a comment
peternewman
left a comment
There was a problem hiding this comment.
Looks great @kripton . Just one minor comment on the NEWS text if you wouldn't mind fixing it, then we can merge this one too!
|
@peternewman Sure, changes to the NEWS file are no problem :) |
Ah yeah, sorry, I re-opened or added it to an existing comment: Do you want to do the honours? |
|
@peternewman Ah, there it is :) |
peternewman
left a comment
peternewman
left a comment
There was a problem hiding this comment.
LGTM, just some new flake8 issues (as flake8 is now a newer version).
I can sort them unless you fancy having a go?
https://travis-ci.org/github/OpenLightingProject/ola/jobs/685997330#L915-L921
ola_rdm_get.py and enforce_licence.py can all become line. Test logger could become s or d to match the others.
| env: | ||
| - TASK='compile' | ||
| - CPPUNIT='1.14' | ||
| - PROTOBUF='latest' |
There was a problem hiding this comment.
Parking that for a later PR.
|
@peternewman : I've made the changes and the unit test cases are still good. Didn't do any further tests as I don't have any RDM-capable interface nor fixtures available here. About that "Parking that for a later PR": Is it something that I should undo or are we good for this PR? |
peternewman
left a comment
peternewman
left a comment
There was a problem hiding this comment.
Sorry some slight confusion on one flake8 change.
Great, sounds good thanks. They're fairly minor changes as you can see (especially as the argument in ola_rdm_get.py isn't even used!
That was regarding expanding the matrix, your change is perfect as it fixes some broken tests on Mac. |
Indeed, that arguments not being used was a bit confusing to me. Changes done, Travis passed 🎉 |
peternewman
left a comment
peternewman
left a comment
There was a problem hiding this comment.
LGTM, thanks for this one too @kripton !
|
Hmm, we may have spoken a bit too soon @kripton . I just went to merge 0.10 back into master, mostly for the Python bits, and found some merge collisions which after a bit of digging allowed me to rediscover #1192 and #1336 so I think the AddDescriptors stuff should ideally be re-edited to be as follows: Are you happy to do that on 0.10 @kripton and do a quick test on your OS? Sorry to keep dragging you into this... |
No problem. Sure, I will have a look 👍 |
|
@peternewman : Done, see #1638 :) |
The ola package (Open Lighting Architecture) version 0.10.2 was removed in commit e692e1f due to an incompatibility with the protobuf version 3.2.0 present in Buildroot at that time. ola was fixed to support newer protobuf version in: OpenLightingProject/ola#1630 This commit reintroduce this package at version 0.10.8. For changelogs since its removal at 0.10.2, see: - https://github.com/OpenLightingProject/ola/releases/tag/0.10.3 - https://github.com/OpenLightingProject/ola/releases/tag/0.10.4 - https://github.com/OpenLightingProject/ola/releases/tag/0.10.5 - https://github.com/OpenLightingProject/ola/releases/tag/0.10.6 - https://github.com/OpenLightingProject/ola/releases/tag/0.10.7 - https://github.com/OpenLightingProject/ola/releases/tag/0.10.8 This commit is based on the previously removed ola package, with the following rework: - Remove the dependency on BR2_HOST_GCC_AT_LEAST_4_5 as host gcc is now guaranteed to be at least 4.8. - Update target gcc dependency to >= 4.8 to reflect protobuf requirement. - Remove the BR2_PACKAGE_OLA_SLP option, which was removed in ola 0.9.4. - Change the "DMX4Linux" plugin option name to "Open DMX" to better reflect the ola option (DMX4Linux is a legacy plugin for 2.6 Kernels). - Update Python support to version 3.x only - Remove patches, as they are no longer needed - Add options for ola plugins: ftdidmx, gpio, karate, openpixelcontrol, renard, spi, uartdmx, usbdmx - Reorder options alphabetically - Update project URL - Add license hashes Signed-off-by: Julien Olivain <[email protected]> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <[email protected]>
3 participants
Same as #1620 but rebased on top of branch "0.10".
See also #1550
Opened as a draft since I didn't test this yet but I would like Travis to build it already.