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

Fixed setup.py and setup.cfg #39

Merged
merged 1 commit into from
Apr 13, 2022

Conversation

KOLANICH
Copy link
Contributor

@KOLANICH KOLANICH commented Feb 3, 2020

No description provided.

Copy link
Contributor

@dgelessus dgelessus left a comment

Choose a reason for hiding this comment

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

LGTM in general. A couple of minor questions though:

setup.cfg Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@KOLANICH KOLANICH force-pushed the setup.cfg branch 3 times, most recently from fb65e81 to f7e0c9b Compare February 13, 2020 11:39
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@GreyCat
Copy link
Member

GreyCat commented Feb 13, 2020

Can you clarify what is the purpose of this PR? There's no description, and title is as ambiguous as it can get:

Fixed setup.py and setup.cfg

What is/was wrong with setup.py/setup.cfg so it required fixing?

@KOLANICH
Copy link
Contributor Author

What is/was wrong with setup.py/setup.cfg so it required fixing?

  1. Obsolete approaches were used. Explicitly reading setup.cfg was no longer needed, it is read and merged automatically even if something is set in setup().
  2. pep517 is a uniform way to trigger building. Implemented its support.
  3. extraction of version without importing. Useful when cwd is an another dir.

Also I wonder if it is acceptible to make kaitaistruct a dir, so we can capture the version from a git tag and save it into version.py using setuptools_scm.

@KOLANICH KOLANICH force-pushed the setup.cfg branch 3 times, most recently from 690a174 to 1d01b14 Compare February 14, 2020 09:34
@higherorderfunctor
Copy link

What is/was wrong with setup.py/setup.cfg so it required fixing?

For me, this fixed being able to install with poetry.

poetry add git+https://github.com/kaitai-io/kaitai_struct_python_runtime.git

[ParseVersionError]
Unable to parse "attr: kaitaistruct.__version__".

I did have to rename the branch though since it is named the same as a file in the repo.

poetry add git+https://github.com/KOLANICH/kaitai_struct_python_runtime.git#setup.cfg

[CalledProcessError]
Command '['git', '--git-dir', '/tmp/pypoetry-git-kaitai_struct_python_runtime5m5xquv5/.git', '--work-tree', '/tmp/pypoetry-git-kaitai_struct_python_runtime5m5xquv5', 'checkout', 'setup.cfg']' returned non-zero exit s
tatus 128.

Clone of this PR with a different branch name.

poetry add git+https://github.com/higherorderfunctor/kaitai_struct_python_runtime.git#setup_fix

Updating dependencies
Resolving dependencies... (4.0s)

Writing lock file


Package operations: 1 install, 0 updates, 0 removals

  - Installing kaitaistruct (0.9 1d01b14)

@KOLANICH
Copy link
Contributor Author

KOLANICH commented Feb 27, 2020

For me, this fixed being able to install with poetry.

please don't use poetry, it is destroying the ecosystem because it is harmful because it is wasteful, but matches well with the current fashionable cargo cult :( It is very sad.

@dgelessus
Copy link
Contributor

@higherorderfunctor Although I don't agree with @KOLANICH's opinion of Poetry, if it fails to parse a setup.cfg with version = attr: ... in it, that's a bug in Poetry and not in the setup.cfg. (I'm actually surprised that Poetry accepts the new setup.cfg from this PR, considering that it still uses the attr: ... feature. Most likely it's interpreting the version as literally attr: __version__, as it looks like Poetry's code for parsing setup.cfg never handles attr: ... anywhere.)

I did have to rename the branch though since it is named the same as a file in the repo.

That is also a bug in Poetry - it's perfectly legal to have a Git branch name that is identical to the name of a file in that branch.

I'd encourage you to report both of these bugs on the Poetry issue tracker: https://github.com/python-poetry/poetry/issues

@KOLANICH KOLANICH force-pushed the setup.cfg branch 2 times, most recently from 2570120 to 135a031 Compare February 27, 2020 22:45
@higherorderfunctor
Copy link

higherorderfunctor commented Feb 28, 2020

I did have to rename the branch though since it is named the same as a file in the repo.

That is also a bug in Poetry - it's perfectly legal to have a Git branch name that is identical to the name of a file in that branch.

I believe you have to add -- to the checkout command to disambiguate file vs branch. Poetry isn't doing this and as far as I know there is no way to inject this (I would agree this is a bug).

https://stackoverflow.com/a/25322508

Copy link
Contributor

@dgelessus dgelessus left a comment

Choose a reason for hiding this comment

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

Well, this PR now does something completely different than before... This isn't "fixing" setup.py and setup.cfg, it's replacing them entirely with a pyproject.toml-based configuration.

Unfortunately, at this point, this is a complete non-starter for us. According to the setuptools documentation about pyproject.toml configuration:

✏️ Note

New in 61.0.0 (experimental)

⚠️ Warning

Support for declaring project metadata or configuring setuptools via pyproject.toml files is still experimental and might change (or be removed) in future releases.

We shouldn't rely on an experimental unstable feature that's not even a month old, unless there's a very good reason for it, and I'm not seeing that here - the current setup.py/setup.cfg work just fine.

Regardless of whether the feature is stable or not, requiring a new setuptools version is also a problem for Python version compatibility. setuptools 61.0.0 requires at least Python 3.7 (59.7.0 dropped support for older Python versions). Kaitai Struct still supports Python 3.4 and 2.7, so we can't require anything newer than setuptools 43.0.0 (from late 2019), the last version to support Python 3.4.

@KOLANICH
Copy link
Contributor Author

KOLANICH commented Apr 9, 2022

(or be removed) in future releases.

feels like bullshit. PEP 621 is accepted, had been long a goal for setuptools and now it is implemented, there is no way back. If you dislike relying on setuptools experimental feature (it contains the bug I have mentioned, that's why version is not dynamic now), I can add 5 lines (2 to enable flit_core as a build backend, 1 a section header, 1 specifying the source file for flit and 1 empty line) and comment out the 2 lines enabling setuptools as a build backend, and the stuff will be built with flit_core, the project section stays the same.

Regardless of whether the feature is stable or not, requiring a new setuptools version is also a problem for Python version compatibility. setuptools 61.0.0 requires at least Python 3.7 (59.7.0 dropped support for older Python versions). Kaitai Struct still supports Python 3.4 and 2.7, so we can't require anything newer than setuptools 43.0.0 (from late 2019), the last version to support Python 3.4.

I wonder if we can backport the latest setuptools to 3.4. Usually python version requirement is raised because of often unjustified (to the point of it being a cargo cult) use of so called f-strings and typing hints for variables within function bodies. I guess it can be possible to write an AST transformer undoing it.

@KOLANICH
Copy link
Contributor Author

KOLANICH commented Apr 9, 2022

Also requires-python specifies the version of a package to run the code on, not the one to build a wheel on.

@generalmimon
Copy link
Member

generalmimon commented Apr 9, 2022

@KOLANICH:

If you dislike relying on setuptools experimental feature (it contains the bug I have mentioned, that's why version is not dynamic now), I can add 5 lines (...), and the stuff will be built with flit_core, the project section stays the same.

I wonder if we can backport the latest setuptools to 3.4. (...) I guess it can be possible to write an AST transformer undoing it.

But why - what would be the point? Why not just use something established and stable? It makes no sense to me.

@KOLANICH
Copy link
Contributor Author

KOLANICH commented Apr 9, 2022

  1. It looks more tidy. I'm a mad perfectionist, you know.
  2. It is more standardized and has more tools around it. And will likely have even more tools thanks to network effects.
  3. setup.cfg is planned to be dropped in indefinite future.

@KOLANICH
Copy link
Contributor Author

KOLANICH commented Apr 9, 2022

Anyway, I revert back to setup.cfg. The metadata can be moved into pyproject.toml in an another PR.

Copy link
Contributor

@dgelessus dgelessus left a comment

Choose a reason for hiding this comment

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

These changes all make sense IMO. Just one suggestion:

pyproject.toml Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@KOLANICH KOLANICH force-pushed the setup.cfg branch 2 times, most recently from 5858f14 to b50f830 Compare April 11, 2022 13:03
@KOLANICH
Copy link
Contributor Author

Why don't you just use the Commit suggestion button that GitHub offers?

Most of JS-based functionality of GitHub is broken for me by now.

Co-Authored-by: dgelessus <dgelessus@users.noreply.github.com>
Copy link
Contributor

@dgelessus dgelessus left a comment

Choose a reason for hiding this comment

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

Thanks!

@dgelessus dgelessus merged commit 36f7ad0 into kaitai-io:master Apr 13, 2022
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.

5 participants