Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Maybe run autogen as part of freetype install #22755

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
May 18, 2022
Merged

Maybe run autogen as part of freetype install #22755

merged 5 commits into from
May 18, 2022

Conversation

wqh17101
Copy link
Contributor

@wqh17101 wqh17101 commented Apr 1, 2022 •
edited
Loading

run autogen.sh before configure freetype
#22754

PR Summary

run autogen.sh before configure freetype

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@wqh17101
Update setupext.py
d09ac16 
run autogen.sh before configure
@QuLogic
Copy link
Member

QuLogic commented Apr 1, 2022

Why? We use a release tarball, so autotools is not necessary.

tacaswell
tacaswell reviewed Apr 1, 2022
View reviewed changes
setupext.py Outdated Show resolved Hide resolved
@tacaswell
Copy link
Member

We do document that the user can find some mechanism to get the source in the right place, I think it is fair to support the "I got this directly from git" case natively (but agree that we should not depend on autotools in general!).

github-actions[bot]
github-actions bot reviewed Apr 1, 2022
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@wqh17101
Copy link
Contributor Author

wqh17101 commented Apr 1, 2022 •
edited
Loading

Why? We use a release tarball, so autotools is not necessary.

You can see my issue. Sometimes you must fix vulnerabilities by patch when it is not released now.@QuLogic

@wqh17101
Copy link
Contributor Author

wqh17101 commented Apr 1, 2022 •
edited
Loading

It seems that it appeared many error in CI progress which is not caused by my PR, so is this the right branch for me to merge to?

@tacaswell
Copy link
Member

The azure windows failures are unrelated, but the rest of the failures are caused by this PR.

If we have a configure then we do not need to run sh autogen.sh which uses autotools internally.

tacaswell
tacaswell requested changes Apr 1, 2022
View reviewed changes
Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only run the autogen script if we need to.

We can not pick up a build-time dependency on autotools.

@wqh17101
Copy link
Contributor Author

wqh17101 commented Apr 1, 2022

Only run the autogen script if we need to.

We can not pick up a build-time dependency on autotools.

You mean trying to build without sh autogen.sh first, and rerun sh autogen.sh and build when there was an exception?

@tacaswell
Copy link
Member

You mean trying to build without sh autogen.sh first,

Yes, or just check if configure exists?

@wqh17101
Copy link
Contributor Author

wqh17101 commented Apr 1, 2022 •
edited
Loading

If we have a configure then we do not need to run sh autogen.sh which uses autotools internally.

You can see the freetype source code , configure and autogen.sh exist at the same time.So i do not think this logic is right.
Also you can see https://gitlab.freedesktop.org/freetype/freetype/-/blob/master/README.git#L36 and https://gitlab.freedesktop.org/freetype/freetype/-/blob/master/.gitlab-ci.yml#L94 , the right way to build freetype from source is running autogen.sh first.

@wqh17101
Copy link
Contributor Author

wqh17101 commented Apr 1, 2022 •
edited
Loading

So i think run autogen.sh will be both right for the source code and release tarball.
On the other hand, the pre-built configuration scripts maybe not suitable for some users who want to build. Generating it in the env of the users is a good solution.

@wqh17101
Copy link
Contributor Author

wqh17101 commented Apr 1, 2022 •
edited
Loading

Oh , i see. you mean that sometimes , the env may not contain the autotools.
So the right logic i think is that running autogen.sh first, and continue if exceptions appear.

@wqh17101
Update setupext.py
774ac2f 
run autogen if you can
tacaswell
tacaswell reviewed Apr 1, 2022
View reviewed changes
setupext.py Outdated Show resolved Hide resolved
@wqh17101
Update setupext.py
f377dc4 
autogen is used to generate configure.ac,so run it if configure.ac not exist.
oscargus
oscargus reviewed Apr 1, 2022
View reviewed changes
setupext.py Outdated Show resolved Hide resolved
@wqh17101 @oscargus
Update setupext.py
23ef7c2 
fix wrong error msg

Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>
wqh17101
wqh17101 commented Apr 1, 2022
View reviewed changes
Copy link
Contributor Author

@wqh17101 wqh17101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed

@oscargus oscargus added status: needs workflow approval For PRs from new contributors, from which GitHub blocks workflows by default. Build labels Apr 1, 2022
@tacaswell tacaswell added this to the v3.6.0 milestone Apr 1, 2022
@wqh17101
Update setupext.py
1fe1cd9 
flask8  line too long
@wqh17101
Copy link
Contributor Author

wqh17101 commented Apr 2, 2022

Although i modified the code for the limitation of max_line_length of flake8, it is better to set max_line_length=100 or 120 for now i think.

@oscargus
Copy link
Member

oscargus commented Apr 5, 2022

Power cycling to restart the tests.

@oscargus oscargus closed this Apr 5, 2022
@oscargus oscargus reopened this Apr 5, 2022
@tacaswell
Copy link
Member

I want to leave final review of this to @QuLogic .

@jklymak jklymak requested a review from QuLogic April 8, 2022 09:07
@jklymak
Copy link
Member

jklymak commented May 18, 2022

ping @QuLogic this is waiting for your review.

@tacaswell tacaswell changed the title Update setupext.py Maybe run autogen as part of freetype install May 18, 2022
@QuLogic QuLogic merged commit 8bfd2c4 into matplotlib:main May 18, 2022
@anntzer anntzer mentioned this pull request May 18, 2022
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oscargus oscargus oscargus left review comments

@github-actions github-actions[bot] github-actions[bot] left review comments

@tacaswell tacaswell tacaswell approved these changes

@QuLogic QuLogic QuLogic approved these changes

Assignees
No one assigned
Labels
Build status: needs workflow approval For PRs from new contributors, from which GitHub blocks workflows by default.
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.
5 participants
@wqh17101 @QuLogic @tacaswell @oscargus @jklymak
Morty Proxy This is a proxified and sanitized view of the page, visit original site.