Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Fix zip bundle structure #27150

Merged
dnfield merged 2 commits into from
Jul 7, 2021
Merged

Fix zip bundle structure #27150

dnfield merged 2 commits into from
Jul 7, 2021

Conversation

@dnfield
Copy link
Contributor

@dnfield dnfield commented Jul 2, 2021

We shouldn't be rebase_pathing the destination.

Before:

Archive:  out/host_debug_unopt/zip_archives/font-subset.zip
Zip file size: 4163109 bytes, number of entries: 3
-rwxr-xr-x  2.0 unx 18887040 b- defN 21-Jul-01 14:16 Users/dnfield/src/flutter/engine/src/flutter/tools/font-subset/font-subset
-rw-r--r--  2.0 unx  1387680 b- defN 21-Jul-02 08:56 Users/dnfield/src/flutter/engine/src/flutter/tools/font-subset/const_finder.dart.snapshot
-rw-r--r--  2.0 unx      427 b- defN 21-Jul-02 08:54 Users/dnfield/src/flutter/engine/src/flutter/tools/font-subset/README.md

After:

Archive:  out/host_debug_unopt/zip_archives/font-subset.zip
Zip file size: 4162733 bytes, number of entries: 3
-rwxr-xr-x  2.0 unx 18887040 b- defN 21-Jul-01 14:16 font-subset
-rw-r--r--  2.0 unx  1387680 b- defN 21-Jul-02 08:56 const_finder.dart.snapshot
-rw-r--r--  2.0 unx      427 b- defN 21-Jul-02 15:12 README.md

@dnfield dnfield requested review from bdero and zanderso July 2, 2021 22:17
@google-cla google-cla bot added the cla: yes label Jul 2, 2021
@dnfield
Copy link
Contributor Author

dnfield commented Jul 2, 2021

Should I try to add some test(s) where we unzip this file and make sure the paths come out right? :\

@zanderso
Copy link
Member

zanderso commented Jul 2, 2021

You could try just inspecting it with https://docs.python.org/3/library/zipfile.html from run_tests.py (or a different script to run from CI).

@dnfield
Copy link
Contributor Author

dnfield commented Jul 7, 2021

Ok, added a test. In doing that I found that I was using Popen.communicate incorrecly for python3, so I updated that as well.

@dnfield dnfield requested a review from cbracken July 7, 2021 21:13
dnfield
dnfield commented Jul 7, 2021
View reviewed changes
tools/font-subset/test.py
cwd=SRC_DIR
)
stdout_data, stderr_data = p.communicate(input=' '.join(codepoints))
stdout_data, stderr_data = p.communicate(input=' '.join(codepoints).encode())
Copy link
Contributor Author

@dnfield dnfield Jul 7, 2021

Choose a reason for hiding this comment

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

.encode is needed for python3.

Copy link
Member

@zanderso zanderso Jul 7, 2021

Choose a reason for hiding this comment

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

You can also pass universal_newlines=True to subprocess.Popen.

Copy link
Contributor Author

@dnfield dnfield Jul 7, 2021

Choose a reason for hiding this comment

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

In addition to .encode, or instead of it?

Copy link
Contributor Author

@dnfield dnfield Jul 7, 2021

Choose a reason for hiding this comment

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

Ahh I see instead of it.

dnfield
dnfield commented Jul 7, 2021
View reviewed changes
tools/font-subset/test.py
with ZipFile(FONT_SUBSET_ZIP, 'r') as zip:
files = zip.namelist()
if 'font-subset%s' % EXE not in files:
print('expected %s to contain font-subset%s' % (files, EXE))
Copy link
Contributor Author

@dnfield dnfield Jul 7, 2021

Choose a reason for hiding this comment

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

This will fail if it got inserted into the zip as Users/me/src/flutter/engine/src/flutter/tools/font-subset instead of just font-subset.

zanderso
zanderso approved these changes Jul 7, 2021
View reviewed changes
Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

lgtm

tools/font-subset/test.py
cwd=SRC_DIR
)
stdout_data, stderr_data = p.communicate(input=' '.join(codepoints))
stdout_data, stderr_data = p.communicate(input=' '.join(codepoints).encode())
Copy link
Member

@zanderso zanderso Jul 7, 2021

Choose a reason for hiding this comment

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

You can also pass universal_newlines=True to subprocess.Popen.

@dnfield dnfield merged commit e838029 into flutter:master Jul 7, 2021
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 8, 2021
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 8, 2021
@engine-flutter-autoroll
e838029 Fix zip bundle structure (flutter/engine#27150)
384b169
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 8, 2021
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 8, 2021
@engine-flutter-autoroll
e838029 Fix zip bundle structure (flutter/engine#27150)
e6305d6
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 8, 2021
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 8, 2021
@engine-flutter-autoroll
e838029 Fix zip bundle structure (flutter/engine#27150)
b717841
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 8, 2021
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 8, 2021
@engine-flutter-autoroll
e838029 Fix zip bundle structure (flutter/engine#27150)
fe37d43
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 8, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 8, 2021
@engine-flutter-autoroll
e838029 Fix zip bundle structure (flutter/engine#27150)
67a2391
moffatman pushed a commit to moffatman/engine that referenced this pull request Aug 5, 2021
@dnfield @moffatman
Fix zip bundle structure (flutter#27150)
86608da 
* Fix zip bundle structure

* Add test, make test.py python3 safe
naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021
@dnfield
Fix zip bundle structure (flutter#27150)
f60e15a 
* Fix zip bundle structure

* Add test, make test.py python3 safe
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@bdero bdero Awaiting requested review from bdero

@cbracken cbracken Awaiting requested review from cbracken

1 more reviewer

@zanderso zanderso zanderso approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dnfield @zanderso