Page MenuHome

Deps: Add potrace as an external library.
ClosedPublic

Authored by Ray molenkamp (LazyDodo) on Aug 20 2020, 6:28 PM.

Details

Summary

The GP team recently gained a dependency on Potrace and while the feature was in development this library lived in /extern

Given this lib is easy to get and there's no blender specific modifications. So there's no reason for it to live in /extern

This diff add potrace as a new dependency in the builder

I tested windows and linux but given linux is not my primary platform a second look would be appreciated there.

I was unable to test on mac (or arm mac)

This diff is a prerequisite for work the GP team would like to land in 2.91

  • Update cmake builder
  • Windows
  • Mac
  • Linux - cmake builder
  • Linux - install deps

Diff Detail

Repository
rB Blender

Event Timeline

Ray molenkamp (LazyDodo) requested review of this revision.Aug 20 2020, 6:28 PM

what is keeping this from getting reviewed ? bcon1 ends in a week, we're cutting it awfully close here.

@Brecht Van Lommel (brecht), I think the process is inverted. What you are asking for is to:

  1. Apply the patch locally.
  2. Run make deps.
  3. Fix possible issues.
  4. Report them here so they are fixed.
  5. Wait for the new round of the patch.
  6. Repeat 2 until issues are solved.
  7. Wait for the patch to be put to master.
  8. Run make deps to make sure latest local patches are applied on the library prior to the library goes to SVN.

This is totally redundant. If you are fine with added extra dependencies, just greenlight the change. The rest platform maintainers will address by direct interaction on the actual code which does exist in Git.
The checklist to track platform statuses is to be separate from the patch, so that its possible to track status after the patch is accepted, committed, and closed.

I should probably have clarified a little. what i wanted out of codereview was:

  1. Do we need/want/prefer this dependency? or perhaps a different solution/lib or a straight up, nope, not doing that!
  2. If we agree this is a dep we'd like to take on, do we like the way it is implemented or would we rather stick it in /extern ?

Once we agree on those basics, whatever platform gets done first can just go ahead and commit, other platforms can build/fix issues at their leisure

I agree making 3-4 different platforms work on the same diff is just gonna annoy everyone

Previously for diffs like this I've updated the patch to fix things on Linux/macOS. If you guys are ok with having temporarily broken make deps in master and then fix it afterwards, that can work as well.

At least from my side, I approve the inclusion of this library, but I have not tested the patch.

This revision is now accepted and ready to land.Sep 8 2020, 7:28 PM

I should also add, changes like this may break not just make deps, but also make.

I believe this patch will break building on macOS, since WITH_POTRACE is enabled by default, but there is no code to detect it in platform_apple.cmake and so also no code to turn it off if not found.

That's fine if @Antonio Vazquez (antoniov) waits to commit code that uses this library until the macOS code for this works, but it gets a little harder to coordinate everything if incomplete patches are committed.

I'm open to a different process here for vetting new libs and existing lib upgrades, the way it has been going has not been super smooth

getting module owners just to say hey ffmpeg/usd/python/randomlib do you want it updated yes/no for 2.90 was like pulling teeth (which is why i didn't start a similar process for 2.91)

I'll happily do the implementation work but a more formalized process to manage the libs wouldn't be the worst thing.

I agree though changes like this the option ought to be "off" until all platforms have caught up (like we did with gmp)

Previously for diffs like this I've updated the patch to fix things on Linux/macOS. If you guys are ok with having temporarily broken make deps in master and then fix it afterwards, that can work as well.

We should probably move this discussion somewhere else.
Anyway, with any change there is a risk that some platform is broken. Even by making things to compile on a specific centOS box there is no guarantee things will work on another Linux flavor. Such fixes are always a reaction to what was already done in master.
If we want to more efficiently ensure that things work for platform maintainers, better approach would be to commit the patch to a branch. It is easier to commit to the branch than to update the revision.

I'm OK with landing this in one of the GP branches (just tell me where it needs to go) so there's actually something to test against (we'll just have to wait with landing it to master until all platforms are done)

Just did a first test of this patch with make and make deps and at least it does not break the build on macOS (Intel).

Tagging @Bastien Montagne (mont29) to keep him in the loop for any possible install_deps updates.