GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Fix wheel generation by Viginox · Pull Request #434 · dephell/dephell

This repository was archived by the owner on Jan 12, 2021. It is now read-only.

Conversation

@Viginox

@Viginox Viginox commented May 13, 2020

Copy link
Copy Markdown
Contributor

Wheel Generation for Packages with src folder

Fixes issue #391

Problem

Packages containing a src folder above the package folder won't have a working wheel package when building the packages with dephell build

Solution

Appending the found package dir to the project package path enables the converter to generate a working wheel package for packages with a src folder as well as for packages without one (when there is no src folder, this parameter contains the value '.' )

Misc

Also fixed a typo in the project_build file.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Viginox added 2 commits May 13, 2020 12:19
@Viginox
Fix wheel generation
90a6af3
Fix issue with non working generation of wheel package for projects with src folder structure
@Viginox
Fix typo
7ae5893
orsinium
orsinium suggested changes May 14, 2020
View reviewed changes

@orsinium orsinium left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Great! That's a good start. As I can see, package_dir keys can have no "" key. For example, if src/__init__.py exists. So, we'll get KeyError in that case.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@Viginox
Fix KeyError when dictionary is empty
b80fd81
@Viginox

Viginox commented May 15, 2020

Copy link
Copy Markdown
Contributor Author

Oh, I totally missed that one 😮

Updated the retrieval of the key from the dict with using the get(...) method and a default value.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

orsinium
orsinium reviewed Jun 2, 2020
View reviewed changes
Comment thread dephell/converters/wheel.py Outdated
path='/'.join(full_path.relative_to(project.package.path).parts),
path='/'.join(
full_path.relative_to(project.package.path.joinpath(
project.package.package_dir.get('',""))).parts),

@orsinium orsinium Jun 2, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sorry, it still doesn't work. .get return an empty string if no match found, and it fails:

from pathlib import Path
Path().absolute().relative_to('')
# ValueError: '/home/gram' does not start with ''

Please, include a few test cases. This should help to see what is possible:
https://github.com/dephell/dephell_discover/blob/master/tests/test_root.py#L42

Thank you for taking care of this. I've done with most of my other projects, so now I'll try to give you feedback faster :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@Viginox Viginox Jun 2, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think I figured out the reason for that. I set the default value to "" (empty) instead of the current directory ".".
I rewrote the code to check if the dictionary is empty and if not, it takes the first value of the dictionary. This is usually only one entry.

I must confess, I don't know where I can write those testcases. You wrote all testcases very generic in the test_all.py file. All other converters only implement testcases for loading.
I try to stick to your scheme of implementing those tests, but at the moment, it is a little bit overwhelming.
Could you give me a hint or an example how to implement the dump test for the wheel package?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@Viginox Viginox Jun 2, 2020

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Taken from your next comment:

"Using the current dir as default is not correct."

If the local folder is not the best default value for that, what would you consider a good alternative?
I think, if the project folder was set for another path, that path should be in that structure nevertheless.

Falling back to the value project.package.path might be a safe choice for that situation.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@Viginox Viginox Jun 3, 2020

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Figured out another approach, which corresponds with how setuptools is generating the wheel package.
The exact approach is described in the commit messages and in the source code itself.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Viginox added 2 commits June 2, 2020 22:03
@Viginox
Update package path generation
f1f1a04
Dynamically create the package path for the wheel generation.
If no keys are in the dictionary, assume path ".", otherwise take the
first element from the dictionary.
@Viginox
Merge remote-tracking branch 'upstream/master' into fix/wheel_generation
1d9b2aa
@orsinium

orsinium commented Jun 2, 2020

Copy link
Copy Markdown
Member

I think you can make a project structure in temp_dir fixture, dump it, and then open it with zipfile and check the dumped structure. Like in the tests I shared above.

Using the current dir as default is not correct. The project can be located somewhere else. I can't say for sure right now, I'm away from the laptop. However, you can make just tests, and I'll help you with the solution. I understand if you lost here, we definitely need more internal documentation. Right now I'm working on type annotations for everything, it should help a bit :)

Off topic: Hmm, why fasthub can't comment on threads?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Viginox

Viginox commented Jun 2, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@orsinium Thanks for the starting point. I'll try to implement some testcases for that.
I think I still need to dig into how to execute the wheel generation properly with as little as possible overhead.

[EDIT] Off topic: Commenting on a thread using fasthub worked for me. Expanded the thread and then there is a separate "Add Commend" filed on the bottom of that thread.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Viginox added 2 commits June 3, 2020 09:21
@Viginox
Update wheel generation
adbfc5d
Make generation of wheel package more robust.

Using the package_dir content to generate the correct folder structure.
This update enables the wheel generation to create the correct file
structure in the wheel package according to the package and package_dir
variables in the setup.py file.
@Viginox
Add proper tests for wheel generation
0451c40
Update the test file with all possible valid settings for the
package_dir file, which can be detected by dephell.
All those different directory structures will produce a valid wheel
file.
@Viginox

Viginox commented Jun 3, 2020

Copy link
Copy Markdown
Contributor Author

While working on this new method to generate the wheel package, I might have encountered a different issue in dephell_discovery, but still some investigation is necessary.
Will look into that, the next days.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Viginox
Fix issues
a591377
* Fix issue zipfile is not supporting pathlib on pypy
* Fix flake8 issues due to not updated local flake8 venv
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@orsinium orsinium orsinium requested changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@Viginox @orsinium
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.