Remove personal workspaces

[benemer]

Switch from Nacho's forks to the original repos. Since the Windows fix for Sophus has not been added, we solve this using a patch.

EDIT: Sorry for the mess @nachovizzo, we were going a bit back and forth 🤣

Now this PR is separating from the others.

[nachovizzo]

What would be the reasoning of this change ?
btw, if we merge this we will release the entire BLAS library in the python packages. The eigen change was there for a reason ;) I think I put the reason somewhere in the comments

[benemer]

What would be the reasoning of this change ? btw, if we merge this we will release the entire BLAS library in the python packages. The eigen change was there for a reason ;) I think I put the reason somewhere in the comments

I saw the comment, but the mentioned link was merged 2 years ago and is also now in version 3.4, see cherry-pick.

In general, Tiziano and I thought it was much cleaner not to use private forks, which also makes it easier to update a dependency in the future.

[nachovizzo]

What would be the reasoning of this change ? btw, if we merge this we will release the entire BLAS library in the python packages. The eigen change was there for a reason ;) I think I put the reason somewhere in the comments

I saw the comment, but the mentioned link was merged 2 years ago and is also now in version 3.4, see cherry-pick.

In general, Tiziano and I thought it was much cleaner not to use private forks, which also makes it easier to update a dependency in the future.

Please don't tell me that Eigen is re-using the same tag and adding changes of top of that 🤣

I do agree with not liking the manual forks, although in the Sophus case, I'm not convinced it's any better. The library stopped development of that version a while ago, and won't evolve. Adding a patch in our repo just to provide windows support seems contaminating. If it would be Linux I'd say it's ok.

[nachovizzo]

What would be the reasoning of this change ? btw, if we merge this we will release the entire BLAS library in the python packages. The eigen change was there for a reason ;) I think I put the reason somewhere in the comments

I saw the comment, but the mentioned link was merged 2 years ago and is also now in version 3.4, see cherry-pick.
In general, Tiziano and I thought it was much cleaner not to use private forks, which also makes it easier to update a dependency in the future.

Please don't tell me that Eigen is re-using the same tag and adding changes of top of that 🤣

I do agree with not liking the manual forks, although in the Sophus case, I'm not convinced it's any better. The library stopped development of that version a while ago, and won't evolve. Adding a patch in our repo just to provide windows support seems contaminating. If it would be Linux I'd say it's ok.

Update, no it's not included in 3.4 release. That's why I manually released a newer version. We DO need that change

Just fetch the release and you will see it https://gitlab.com/libeigen/eigen/-/archive/3.4.0/eigen-3.4.0.zip

[benemer]

Please don't tell me that Eigen is re-using the same tag and adding changes of top of that 🤣

Yes it looks like it 🤣

The library stopped development of that version a while ago, and won't evolve. Adding a patch in our repo just to provide windows support seems contaminating. If it would be Linux I'd say it's ok.

I actually prefer the patch because it shows what has to be changed on top of that Sophus version. Otherwise, one needs to fetch the link to your Sophus fork, go to the stale branches and then see that there is a one line change and it's 19 commits behind. But this is probably personal preference.

Update, no it's not included in 3.4 release. That's why I manually released a newer version. We DO need that change

Yes, that's why we use the git repo and check out at 3.4.0 and do not fetch the tar ball. They kept adding stuff to the 3.4.0 branch which is not in the release.

[nachovizzo]

Please don't tell me that Eigen is re-using the same tag and adding changes of top of that 🤣

Yes it looks like it 🤣

Holy moly

I actually prefer the patch because it shows what has to be changed on top of that Sophus version. Otherwise, one needs to fetch the link to your Sophus fork, go to the stale branches and then see that there is a one line change and it's 19 commits behind. But this is probably personal preference.

Ok, you have a point. It's true, that I might see that and think "this guys have a shady branch with who knows what", and with the patch is visible we haven't changed anything. Maybe worth applying the patch for windows only then ?

Yes, that's why we use the git repo and check out at 3.4.0 and do not fetch the tarball. They kept adding stuff to the 3.4.0 branch which is not in the release.

mmm, but this will inject git as a build dependency, something we decided not to do many years ago, and that's why we switched to tarballs. I's a matter of design. A 3rd option would be to also add that as patch

[benemer]

Maybe worth applying the patch for windows only then ?

Can we do that in a nice way?

but this will inject git as a build dependency

Yes true, that's the downside. But how relevant is it not to have git as a build dependency?

A 3rd option would be to also add that as patch

Agree, but then we miss a lot of (possible) bugfixes for 3.4 and I am not sure how relevant they are.

[benemer]

So there are three options now:

1. Keep private forks
2. Introduce git as a dependency, get the "latest" Eigen 3.4 and Sophus + patch
3. Fetch Eigen and Sophus tarballs and patch both

@tizianoGuadagnino what do you think?

[nachovizzo]

Maybe worth applying the patch for windows only then ?

Can we do that in a nice way?

but this will inject git as a build dependency

Yes true, that's the downside. But how relevant is it not to have git as a build dependency?

A 3rd option would be to also add that as patch

Agree, but then we miss a lot of (possible) bugfixes for 3.4 and I am not sure how relevant they are.

1. I don't think so
2. How relevant ? we can't tell now. I think when we discussed this with @tizianoGuadagnino ages ago we were both convinced about not having git as build dependency. But if the CI is passing then it shouldn't be a problem. On the other hand, in our robots, we don't have git, and I believe this is true for many other people.
3. Yeah but that argument holds true for all libraries in the world. Those bugfixes probably are not affecting us. And it's likely that you are even using an older version of ubuntu anyways (you, me, and everyone that has eigen installed)

[tizianoGuadagnino]

So there are three options now:

1. Keep private forks
2. Introduce git as a dependency, get the "latest" Eigen 3.4 and Sophus + patch
3. Fetch Eigen and Sophus tarballs and patch both

@tizianoGuadagnino what do you think?

(3) is the way to go in my opinion as this will remove both problems:

1. No shady private repos
2. no git as dependency so that this can be build on a machine without git (e.g. NUC on a robot)

[tizianoGuadagnino]

Maybe worth applying the patch for windows only then ?

Can we do that in a nice way?

but this will inject git as a build dependency

Yes true, that's the downside. But how relevant is it not to have git as a build dependency?

A 3rd option would be to also add that as patch

Agree, but then we miss a lot of (possible) bugfixes for 3.4 and I am not sure how relevant they are.

1. I don't think so
2. How relevant ? we can't tell now. I think when we discussed this with @tizianoGuadagnino ages ago we were both convinced about not having git as build dependency. But if the CI is passing then it shouldn't be a problem. On the other hand, in our robots, we don't have git, and I believe this is true for many other people.
3. Yeah but that argument holds true for all libraries in the world. Those bugfixes probably are not affecting us. And it's likely that you are even using an older version of ubuntu anyways (you, me, and everyone that has eigen installed)

For me the big argument that @nachovizzo is pointing out is the capability to build on robots, which we cannot compromise on.

[nachovizzo]

Ok, I think we all agree that we patch the original library versions so changes are visible but without relying on git.

Nice addition!

[benemer]

Okay, so now we fetch Sophus and Eigen tarballs and patch both of them not using git. Everything is from public repos, and everyone can follow what has to be patched.

Note that the Eigen version is now slightly different from before. Before, we used 3.4.90 from Nacho's private fork, which is basically branching off from this commit on Eigen master and applies the patch. Now, we use the original 3.4.0 release and apply the patch. But as Nacho said before, these minor version differences should not matter.

[tizianoGuadagnino]

@nachovizzo I wait you pull the trigger to approve and merge.

I am loving these changes btw

[nachovizzo]

For the sake of records, and to avoid wasting time yet again on this in the future.

I can't recall why I particularly added that patch in eigen (good job on commit messages btw) but I do remember the BLAS library being built in the background without any need. For that I forked eigen and used the version that allows us to set the build of BLAS to off, otherwise, it doesn't make sense to add such a particular change.

I don't have a Windows machine at hand, but this whole thing originated in this PR where they report

Because, my Windows machine had Fortran on it the cross compile would try to Build BLAS and LPACK even though I did not have a valid cross compiler for Fortran.

Which is extremely similar to what I remember. I think is better to make sure we do have the BLAS build disabled without having to rely on weird combinations of cmake and star alliance to bet that we will not build that library.

[nachovizzo]

strasdat/Sophus#436 (comment)

Good news. We can probably now get also rid of the patch