Move include of Python.h to fix Arch build issue #391
Conversation
emersonknapp
changed the title
|
@seedship this looks good to me, but it looks like it has a merge conflict, can you rebase to master? |
Signed-off-by: Richard Nai <richardnai6@gmail.com>
|
@emersonknapp Ahh my bad, I meant to merge into the eloquent branch. Updated the base to reflect this. |
|
Is this change also required on master to build Foxy on Arch? Happy to merge this fix into eloquent regardless |
|
Hm I'm not sure, I tried to get the Foxy source by doing wget https://raw.githubusercontent.com/ros2/ros2/foxy/ros2.repos but it returned a 404. |
|
The Foxy branch hasn't been cut there yet, it's still on |
|
Ah I just checked, rosbag2 built fine on my machine in master |
|
Excellent. Trying a Eloquent build here - think I got all the params right
|
|
Hm, why did the builds fail? They built fine on my machine |
|
Ah - I did a |
|
|
Re-run windows |
|
@Karsten1987 looping you in - i haven't done patchfixes for released distros, is there any process beyond merging into the relevant branch? |
|
Normally, I'd recommend to open the initial PR towards the master branch. Once that PR gets merged, we've cherry-picked that commit on top of the release branches. That would make it a bit more straight forward - technically that commit could also be cherry-picked onto dashing - it's still a support distro. Either way, we have to mark this PR within the patch release project board, so that we cut a new release. |
|
Trying another rerun Re-run windows |
|
I've tabled this for the patch release. It should be no problem to later on backport this to Foxy, once the windows build problems are sorted out. |
|
Looks good to me. Please enhance this change though with a small comment saying why this is needed.
|
|
@seedship can you please rebase your branch? That will unblock CI. |
|
Sorry for the long delay. I have just checked the eloquent branch, and I see no new commits have been made. Do I need to rebase my master branch? I'm trying to merge into eloquent. |
|
Please rebase on top of master and target the PR towards the master branch. I would like to make sure that we don't break anything on master and will then come up with a backport of that commit for foxy and eloquent. |
|
I have just rebased my master branch. However, it is worth noting that master builds successfully on my computer, without the commit. Only Eloquent doesn't build successfully without the reordering, and I don't know why. |
|
|
@seedship can you verify that your branch sits correctly on top of master? Looks like CI is still failing due to some missing commits (removal of poco in particular). But given that it compiles correctly on foxy/master, I am wondering if that is the correct fix though - Apologies, I haven't read the ongoing thread here completely before. This looks more like calming the symptoms than fixing what's actual going wrong here. But I also don't really know how to approach this. @emersonknapp what do you think? Should be instead just merge this straight onto eloquent instead of master and then backporting it? |
|
Since master/Foxy is currently working, i think it makes more sense to just put the patchfix straight onto Eloquent. I don't know of a specific commit that we could backport. |
|
Eloquent run:
|
There was a problem hiding this comment.
This looks good to and looking at the "unstable" results. That's either due to the fact that I actually ran CI on focal as well as the typical end-to-end tests which were addressed later on (master branch).
@emersonknapp I'll leave it up to to merge.
|
Just for my own peace of mind I'm going to try the build on bionic as well
|
|
Thanks @seedship for the patch - thanks for the patience! |
|
My pleasure, thanks! |
It sounds like this patch is not intended for Foxy, so I'm removing this from the project board, please re-add if I'm mistaken. |
The include of Python.h needs to be rearranged. It cannot be imported first because it redefines _XOPEN_SOURCE
See issue #383