FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Maybe run autogen as part of freetype install by wqh17101 · Pull Request #22755 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Maybe run autogen as part of freetype install - #22755

Merged
QuLogic merged 5 commits into
matplotlib:mainfrom
wqh17101:issue-22754
May 18, 2022
ConversationCommits (5)ChecksFiles changed
Merged

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

Conversation

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).

Update setupext.py
d09ac16 
run autogen.sh before configure

QuLogic commented Apr 1, 2022

Copy link
Copy Markdown
Member

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

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

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 reviewed Apr 1, 2022
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 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 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 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 requested changes Apr 1, 2022
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Only run the autogen script if we need to.

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

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 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 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 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 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.

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

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

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

changed

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

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 commented Apr 5, 2022

Copy link
Copy Markdown
Member

Power cycling to restart the tests.

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

tacaswell commented Apr 5, 2022

Copy link
Copy Markdown
Member

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

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

jklymak commented May 18, 2022

Copy link
Copy Markdown
Member

ping @QuLogic this is waiting for your review.

tacaswell changed the title Update setupext.py Maybe run autogen as part of freetype install May 18, 2022
QuLogic merged commit 8bfd2c4 into matplotlib:main May 18, 2022
anntzer mentioned this pull request May 18, 2022
Merged
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

oscargus oscargus left review comments
github-actions[bot] github-actions[bot] left review comments
tacaswell tacaswell approved these changes
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

Back | FazBrowse Home | New Git URL