Skip to content

Add necessary fixes to make new releases#9

Merged
ViolanteCodes merged 1 commit intomasterfrom
fix/build
Apr 24, 2024
Merged

Add necessary fixes to make new releases#9
ViolanteCodes merged 1 commit intomasterfrom
fix/build

Conversation

@martinalbert
Copy link
Contributor

@martinalbert martinalbert commented Apr 23, 2024

This PR adds necessary fixes to make new releases possible.

  1. Adds requirements.txt for defining required dependencies (for command pip install -r requirements.txt)
  2. Gets version from __version__.py file with use of exec - avoiding initialization of the package prematurely
  3. Fixes typo in imported exception HTTPError from package requests

@martinalbert martinalbert changed the title fix: Add requirements and version reading Add necessary fixes to make new releases Apr 23, 2024
@Toreno96
Copy link

  1. Adds reading of __version__.py file avoiding initialization of the package prematurely

@martinalbert It's not clear to me why this is needed, could you elaborate?

@martinalbert
Copy link
Contributor Author

@Toreno96 sure, the change regarding version is required mainly due to how python module pypa/build for building is working. Using older, deprecated, tool bdist_wheel (python setup.py sdist bdist_wheel) which does not throw an error when importing version like it was before this change, should be avoided and for building we should use pypa/build. I don’t know exactly how differently they work, but it has something to do with how these tools initialize and load package setup.

@martinalbert martinalbert requested a review from Toreno96 April 24, 2024 09:42
@Toreno96
Copy link

the change regarding version is required mainly due to how python module pypa/build for building is working. Using older, deprecated, tool bdist_wheel (python setup.py sdist bdist_wheel) which does not throw an error when importing version like it was before this change, should be avoided and for building we should use pypa/build. I don’t know exactly how differently they work, but it has something to do with how these tools initialize and load package setup.

@martinalbert Do you mean that pypa/build is not able to use a version imported from a module like bdist_wheel? Am I getting it right?

A loose idea, but what if we try to go extra simple and define the version inline in the setup.py itself, instead of having a separate file and hacking its content into the setup.py? What could be the possible cons of that?

@martinalbert
Copy link
Contributor Author

@Toreno96 Yes, when using pypa/build and imported version from a module, it probably tries to automatically find other needed dependecies (even though version file is not using any), which are at the time of setup.py not needed nor available.

I responded here regarding the version in setup.py.

- Add `requirements.txt` for defining required dependencies
- Get version from `__version__.py` file with exec - avoiding initialization of the package prematurely
- Fix typo in imported exception HTTPError from package requests
Copy link

@Toreno96 Toreno96 left a comment

Choose a reason for hiding this comment

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

LGTM, I don't have any more comments

cc @ViolanteCodes

Copy link
Contributor

@ViolanteCodes ViolanteCodes left a comment

Choose a reason for hiding this comment

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

This builds for me - I was able to setup a new environment and do a clean install and python setup.py sdist bdist_wheel without issue.

@ViolanteCodes ViolanteCodes merged commit 426e265 into master Apr 24, 2024
@ViolanteCodes ViolanteCodes deleted the fix/build branch April 24, 2024 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants