Skip to content
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.

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

Fix wheel packages with _ in their name not being converted to - #555

Closed

Conversation

stealthycoin
Copy link
Contributor

Since wheel files use - as a delimeter between the different
components of the filename such as distribution-version-... if the
distribution has - in it they are converted to _. So when we read a
wheel file that has _ in the distribution, they need to be remaped to
-.

cc @jamesls @kyleknap @dstufft

@codecov-io
Copy link

codecov-io commented Sep 26, 2017

Codecov Report

Merging #555 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #555      +/-   ##
==========================================
+ Coverage   94.43%   94.44%   +<.01%     
==========================================
  Files          18       18              
  Lines        3127     3130       +3     
  Branches      402      402              
==========================================
+ Hits         2953     2956       +3     
  Misses        126      126              
  Partials       48       48
Impacted Files Coverage Δ
chalice/deploy/packager.py 95.73% <100%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 754dbe9...6e21a7d. Read the comment docs.

@jamesls
Copy link
Member

jamesls commented Sep 26, 2017

Can you update your commit message with specific packages that have this issue? It'd be great just to have some real world data here.

@@ -499,6 +499,7 @@ def _calculate_name_and_version(self):
# {distribution}-{version}(-{build tag})?-{python tag}-{abi tag}-
# {platform tag}.whl
name, version = self.filename.split('-')[:2]
name = name.replace('_', '-')
Copy link

Choose a reason for hiding this comment

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

It might be smart to use the PEP 503 normalization rules here, or at least in the __eq__() implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ill update the commit and look at PEP 503

Since wheel files use - as a delimeter between the different
components of the filename such as distribution-version-... if the
distribution has - in it they are converted to _. So when we read a
wheel file that has _ in the distribution, they need to be remaped to
-. An example of a package that has this issue is
`googleapis-common-protos`.
Add an integration test for a package with hyphens in the name. Changed
the renaming to follow PEP 503 rather than just replacing _ with -.
f.write('%s\n' % package)
cli_factory = factory.CLIFactory(app_skeleton)
package_output_location = os.path.join(app_skeleton, 'pkg')
print(package_output_location)
Copy link
Member

Choose a reason for hiding this comment

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

Remove print statement

'debug': False,
'factory': cli_factory})
assert result.exit_code == 0
assert result.output.strip() == 'Creating deployment package.'
Copy link
Member

Choose a reason for hiding this comment

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

Can we actually assert that the package dir looks correct? It doesn't have to be detailed, but this test would pass with code that literally did print "Creating deployment pakage."

Copy link
Member

@jamesls jamesls left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants