Movatterモバイル変換

Copy link

github-actionsbot 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 uson gitter for real-time discussion.

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

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

Copy link

Member

tacaswell commentedFeb 9, 2022

Why does ARM require freetype 2.11.0?

I would quible a bit with the wording of "support", we have always supported newer versions of freetype (and the linux distributions regularly ship Matplotlib with newer versions), but due to changes in the text layout, many of our tests will fail if you do not user FT2.6.1.

Copy link

ContributorAuthor

niyas-sait commentedFeb 9, 2022

Why does ARM require freetype 2.11.0?

Freetype cannot be built for arm64 for earlier versions. MSBuild configurations were missing.

I would quible a bit with the wording of "support", we have always supported newer versions of freetype (and the linux distributions regularly ship Matplotlib with newer versions), but due to changes in the text layout, many of our tests will fail if you do not user FT2.6.1.

Yes, Freetype 2.11.0 seems to generate libraries in a slightly different place, and had to tweak the matplotlib scripts a bit to fix it. That's why I have used the wording supported :-) Happy to change it

Copy link

ContributorAuthor

niyas-sait commentedFeb 14, 2022

@tacaswell CI failures seem to be unrelated (looks like a known issue around ghost scripts based on the discussion on gitter). Could you review the patch please?

niyas-sait force-pushed thewindows-arm64 branch from1f3e178 tof394b0bCompare

February 15, 2022 12:10

tacaswell reviewed

Feb 17, 2022

lib/matplotlib/__init__.py Outdated

		@@ -1199,7 +1200,9 @@ def is_interactive():
		def _init_tests():
		# The version of FreeType to install locally for running the
		# tests. This must match the value in `setupext.py`
		LOCAL_FREETYPE_VERSION = '2.6.1'
		win_arm64 = sys.platform.startswith('win') and \

Copy link

Member

tacaswellFeb 17, 2022

Choose a reason for hiding this comment

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

Can you drop this change? Even if the default freetype we will try to build on arm64 in 2.11.1, it is still true that the tests will fail (due to changes in the font rendering) so we should still provide this warning.

I suspect that this (local) variable should have been calledVERSION_OF_FREETYPE_THE_TESTS_WORK_WITH ;)

Copy link

ContributorAuthor

niyas-saitFeb 17, 2022

Choose a reason for hiding this comment

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

Yes that make sense 👍

Copy link

ContributorAuthor

niyas-saitFeb 17, 2022

Choose a reason for hiding this comment

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

Pushed a commit to revert that part.

tacaswell reviewed

Feb 17, 2022

setupext.py OutdatedShow resolvedHide resolved

oscargus added status: needs workflow approval

For PRs from new contributors, from which GitHub blocks workflows by default.

OS: Microsoft labels

Feb 17, 2022

tacaswell added this to thev3.5.2 milestone

Copy link

Member

tacaswell commentedFeb 18, 2022

@QuLogic Any reason to not pull this back for 3.5.2?

tacaswell reviewed

setupext.py OutdatedShow resolvedHide resolved

tacaswell approved these changes

anntzer reviewed

lib/matplotlib/__init__.py OutdatedShow resolvedHide resolved

anntzer reviewed

setupext.py OutdatedShow resolvedHide resolved

oscargus reviewed

lib/matplotlib/__init__.py OutdatedShow resolvedHide resolved

niyas-saitand others added8 commits

February 18, 2022 11:09

Enable windows arm64 targets

c32dcc8

- Add FreeType 2.11.1 support- Add MSBuild platform selection for ARM64- Add support for static builds for FreeType versions 2.10 and above

refactor to skip codecov

df7f8c0

fix linting errors

5d79ce5

revert changes in lib/matplotlib/__init__.py

5595d34

Update setupext.py

1f54d47

Co-authored-by: Thomas A Caswell <tcaswell@gmail.com>

STY: remove trailing whitespace

b509068

Minor clean up to keep the line length bit short

76d2e02

restore empty line missed in __init__.py

af3a7af

niyas-sait force-pushed thewindows-arm64 branch froma5b4592 toaf3a7afCompare

February 18, 2022 11:10

Copy link

ContributorAuthor

niyas-sait commentedFeb 18, 2022

codecov tests don't seem happy now! It is not flagging any particular lines. Could it be a rebase issue?

minor refactoring

08cb182

Copy link

Member

tacaswell commentedFeb 18, 2022

codecov can lag a bit based on when CI jobs finish. We need all of the jobs to get the full coverage, but codecov will update the issue when it gets most of them.

oscargus approved these changes

Feb 22, 2022

Copy link

Member

QuLogic commentedFeb 25, 2022

Freetype cannot be built for arm64 for earlier versions. MSBuild configurations were missing.

What exactly is the failure? We don't use MSBuild to build bundled FreeType.

Copy link

ContributorAuthor

niyas-sait commentedFeb 25, 2022

Freetype cannot be built for arm64 for earlier versions. MSBuild configurations were missing.
What exactly is the failure? We don't use MSBuild to build bundled FreeType.

Previous FreeType versions didn't have the ARM64 specific build configurations.

Don't we usefreetype.sln to build FreeType as part of building extensions ? (ref:setupext.py, line 594, do_custom_build)

Copy link

Member

QuLogic commentedFeb 25, 2022

No, you're right, I forgot that happens on Windows.

Copy link

ContributorAuthor

niyas-sait commentedMar 8, 2022

Can this be merged?

tacaswell reviewed

doc/users/next_whats_new/windows_arm64.rst OutdatedShow resolvedHide resolved

DOC: tweak tense

a0cb369

tacaswell reviewed

setupext.py OutdatedShow resolvedHide resolved

tacaswell reviewed

setupext.py OutdatedShow resolvedHide resolved

tacaswell reviewed

setupext.py OutdatedShow resolvedHide resolved

Copy link

Member

tacaswell commentedMar 10, 2022

@nsait-linaro I do not fully understand what is going on at the bottom? It looks like it will grab the last path that last path that matches? Do we expect there to ever be more than one path that matches and/or can that path be directly constructed again?

niyas-sait added2 commits

March 10, 2022 20:34

Minor clean up and refactoring for freetype lib finding part

4865259

remove next and do a simple list unpacking

e773f93

Copy link

ContributorAuthor

niyas-sait commentedMar 10, 2022

@nsait-linaro I do not fully understand what is going on at the bottom? It looks like it will grab the last path that last path that matches? Do we expect there to ever be more than one path that matches and/or can that path be directly constructed again?

@tacaswell There should be only one match as we are building only specific platform at a time. I have done a bit of refactoring to reflect that.

tacaswell reviewed

setupext.py

		msbuild_platform = (
		'x64' if platform.architecture()[0] == '64bit' else 'Win32')
		base_path = Path("build/freetype-2.6.1/builds/windows")
		is_x64 = platform.architecture()[0] == '64bit'

Copy link

Member

tacaswellMar 10, 2022

Choose a reason for hiding this comment

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

This is much clearer 👍🏻

tacaswell approved these changes