Default to local_freetype builds. #15476
Conversation
|
I would still prefer a switch in cfg file instead of environment variable, but I can live with that. Would it be appropriate to use the cfg switch only and just remove the environment variable? |
73bc5a4 to
1f2f47d
Compare
|
I have a mild preference for the environment variable (there's less friction setting/unsetting it from the command line than opening a config file, editing it, and saving) but can switch to the cfg approach if the consensus is on that side. Any reason for preferring it? |
|
It looks to me that all the other environment variables are mostly used for runtime, while setup.cfg switches are mostly for the build script. The first comment in setup.py also states "The matplotlib build options can be modified with a setup.cfg file" and does not mention environment variables. It would be more consistent if the freetype build option keeps that way. |
|
Fair enough. Let's give some time for others to chime in (the implementation is not the problem here :)). |
|
I'm probably +0 on using setup.cfg (mostly because in debian we already do :) ) what i'm mildly concerned about is what's gonna happen with the baseline images and unittests : did you look into the impact that may have and how to address it? |
|
The situation is unchanged for baseline images: you still need to have a ft2.6.1 build for baseline images to match exactly; as Debian uses (I guess) a more recent ft you'll need to keep whatever patch you are using to make tests pass (likely, raising the tolerance of image comparison tests). |
|
Regarding that last point, as said in #14170 (comment) and following comments, skipping the baseline image tests could also be a possibility (I’m not sure relaxed tests are way better than no tests). And obviously I agree with @felixonmars that keeping everything in setup.cfg makes more sense. |
1f2f47d to
c81cc3d
Compare
|
OK, I switched to a libs.system_freetype entry in setup.cfg. (I moved it out of |
This would make builds from source work straightforwardly (including for testing purposes) and simplify the build instructions for new contributors -- see changes in build instructions. Note that there is intentionally no corresponding environment variable because it is unclear whether e.g. a set but *empty* MPLSYSTEMFREETYPE environment variable should override a system_freetype = True in setup.cfg, and because this would make the error message in checkdep_freetype.c more complex (do we tell the use to unset the environment variable? or should setting it to "0" or "false" be the same as unsetting it?). The design here keeps things simple by having a single "on" switch. The config entry is under [libs] to allow for a future system_qhull entry.
c81cc3d to
c092ecd
Compare
|
We should also get feedback from @cgohlke @msarahan and track down someone from brew. Given that this will make it much easier for new contributors to get a working build I am 👍. It does put a bit more burden on our downstream packagers, but it is likely they are maintaining patches / some scripts around us anyway so I don't expect it to be be a huge burden (but they will need to know it's coming!) so that is a -0 for me. |
|
You're 👍 but -0? :) |
|
Seems fine to me. We can set env vars or patch setup.cfg appropriately. No preference. Thanks for the heads-up @tacaswell |
|
That's ok with me. |
|
@anntzer sorry for not being clear. On one concern I am 👍 on the other I am -0 which is still net 👍 . |
This would make builds from source work straightforwardly (including
for testing purposes) and simplify the build instructions for new
contributors -- see changes in build instructions.
Note that there is intentionally no corresponding entry in setup.cfgbecause it is unclear whether e.g. a set but empty MPLSYSTEMFREETYPE
environment variable should override a hypothetical system_freetype =
True in setup.cfg, and because this would make the error message in
checkdep_freetype.c more complex (do we tell the use to unset the
environment variable? or should setting it to "0" or "false" be the
same as unsetting it?). The design here keeps things simple by having a
single "on" switch.
Note that there is intentionally no corresponding environment variable
because it is unclear whether e.g. a set but empty MPLSYSTEMFREETYPE
environment variable should override a system_freetype = True
in setup.cfg, and because this would make the error message in
checkdep_freetype.c more complex (do we tell the use to unset the
environment variable? or should setting it to "0" or "false" be the same
as unsetting it?). The design here keeps things simple by having a
single "on" switch.
The config entry is under [libs] to allow for a future system_qhull
entry.
As suggested in #11984 (comment) we could have a similar switch for libqhull.
attn @QuLogic @sandrotosi @felixonmars @ArchangeGabriel (the linux packagers).
PR Summary
PR Checklist