Skip to content

Maybe run autogen as part of freetype install - #22755

Merged
QuLogic merged 5 commits into
matplotlib:mainfrom
wqh17101:issue-22754
May 18, 2022
Merged

Maybe run autogen as part of freetype install#22755
QuLogic merged 5 commits into
matplotlib:mainfrom
wqh17101:issue-22754

Conversation

@wqh17101

@wqh17101 wqh17101 commented Apr 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

QuLogic commented Apr 1, 2022

Copy link
Copy Markdown
Member

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

tacaswell
tacaswell reviewed Apr 1, 2022
View reviewed changes
Comment thread setupext.py Outdated
@tacaswell

tacaswell commented Apr 1, 2022

Copy link
Copy Markdown
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

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

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

wqh17101 commented Apr 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

wqh17101 commented Apr 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

tacaswell commented Apr 1, 2022

Copy link
Copy Markdown
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

@tacaswell tacaswell left a comment

Copy link
Copy Markdown
Member

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

wqh17101 commented Apr 1, 2022

Copy link
Copy Markdown
Contributor Author

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

tacaswell commented Apr 1, 2022

Copy link
Copy Markdown
Member

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

Yes, or just check if configure exists?

@wqh17101

wqh17101 commented Apr 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

wqh17101 commented Apr 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

wqh17101 commented Apr 1, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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
Comment thread setupext.py Outdated
@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
Comment thread setupext.py Outdated
@wqh17101 @oscargus
Update setupext.py
23ef7c2 
fix wrong error msg

Co-authored-by: Oscar Gustafsson <[email protected]>
wqh17101
wqh17101 commented Apr 1, 2022
View reviewed changes

@wqh17101 wqh17101 left a comment

Copy link
Copy Markdown
Contributor Author

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

wqh17101 commented Apr 2, 2022

Copy link
Copy Markdown
Contributor Author

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

oscargus commented Apr 5, 2022

Copy link
Copy Markdown
Member

Power cycling to restart the tests.

@oscargus oscargus closed this Apr 5, 2022
@oscargus oscargus reopened this Apr 5, 2022
@tacaswell

tacaswell commented Apr 5, 2022

Copy link
Copy Markdown
Member

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

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

jklymak commented May 18, 2022

Copy link
Copy Markdown
Member

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