Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Default to local_freetype builds.#15476
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
felixonmars commentedOct 22, 2019
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 to1f2f47dCompareanntzer commentedOct 22, 2019
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? |
felixonmars commentedOct 22, 2019
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. |
anntzer commentedOct 22, 2019
Fair enough. Let's give some time for others to chime in (the implementation is not the problem here :)). |
sandrotosi commentedOct 22, 2019
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? |
anntzer commentedOct 22, 2019
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). |
ArchangeGabriel commentedOct 22, 2019
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 toc81cc3dCompareanntzer commentedOct 22, 2019
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 (includingfor testing purposes) and simplify the build instructions for newcontributors -- see changes in build instructions.Note that there is intentionally no corresponding environment variablebecause it is unclear whether e.g. a set but *empty* MPLSYSTEMFREETYPEenvironment variable should override a system_freetype = Truein setup.cfg, and because this would make the error message incheckdep_freetype.c more complex (do we tell the use to unset theenvironment variable? or should setting it to "0" or "false" be the sameas unsetting it?). The design here keeps things simple by having asingle "on" switch.The config entry is under [libs] to allow for a future system_qhullentry.
c81cc3d toc092ecdComparetacaswell commentedOct 22, 2019
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. |
anntzer commentedOct 22, 2019
You're 👍 but -0? :) |
msarahan commentedOct 23, 2019
Seems fine to me. We can set env vars or patch setup.cfg appropriately. No preference. Thanks for the heads-up@tacaswell |
cgohlke commentedOct 23, 2019
That's ok with me. |
tacaswell commentedOct 24, 2019
@anntzer sorry for not being clear. On one concern I am 👍 on the other I am -0 which is still net 👍 . |
Uh oh!
There was an error while loading.Please reload this page.
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 butempty 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 butempty 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