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

Migrate to PEP 518 #1111

Closed
wants to merge 11 commits into from

Conversation

nullableVoidPtr
Copy link
Contributor

Checklist

Which kind of PR do you create?

  • This PR only contains minor fixes.
  • This PR contains major feature update.
  • This PR introduces a new function/api for Qiling Framework.

Coding convention?

  • The new code conforms to Qiling Framework naming convention.
  • The imports are arranged properly.
  • Essential comments are added.
  • The reference of the new code is pointed out.

Extra tests?

  • No extra tests are needed for this PR.
  • I have added enough tests for this PR.
  • Tests will be added after some discussion and review.

Changelog?

  • This PR doesn't need to update Changelog.
  • Changelog will be updated after some proper review.
  • Changelog has been updated in my PR.

Target branch?

  • The target branch is dev branch.

One last thing


This PR makes the qiling package removes the setup.py script and replaces it with the declarative setup.cfg file, and ensures that it is compatible with the new pyproject.toml meta build system specified in PEP 518, as discussed in #1109.

Most of the package metadata including dependencies are included in anticipation of PEP 621/631 support in setuptools (see pypa/setuptools#1688 and pypa/setuptools#2970). Alternatively, Qiling can use https://github.com/pypa/flit) to rely entirely on the one TOML file, and I believe pip would work just the same.

@xwings
Copy link
Member

xwings commented Mar 23, 2022

Erm, now we need to main two dependency?

@nullableVoidPtr
Copy link
Contributor Author

Ah, sorry if I didn't make it clear.

The list of runtime dependencies is the same, but it is duplicated twice to conform to a finalized PEP, which would eventually be supported in setuptools.

I brought up filt as an optional alternative, which maintainers could choose to use, but for now, in order to make this PR work with the current setuptools version (what everyone uses), I have specified dependencies in both the standard project TOML, and the declarative setuptools config file. This would get rid of the deprecated setup.py process.

There should be little/no change to how contributors develop.

@xwings
Copy link
Member

xwings commented Mar 31, 2022

@elicn what do you think ?

@elicn
Copy link
Member

elicn commented Mar 31, 2022

I think that keeping up to date is always a good idea; see my full feddback at #1109.
Since I don't deal with the release and distribution processes I don't know whether this is going to add more burden or not. I guess it is for you to consider and decide.

@xwings
Copy link
Member

xwings commented Apr 16, 2022

Ah, sorry if I didn't make it clear.

The list of runtime dependencies is the same, but it is duplicated twice to conform to a finalized PEP, which would eventually be supported in setuptools.

I brought up filt as an optional alternative, which maintainers could choose to use, but for now, in order to make this PR work with the current setuptools version (what everyone uses), I have specified dependencies in both the standard project TOML, and the declarative setuptools config file. This would get rid of the deprecated setup.py process.

There should be little/no change to how contributors develop.

I still dont understand where they are two files. one is TOML and one setuptools ?

@nullableVoidPtr
Copy link
Contributor Author

I still dont understand where they are two files. one is TOML and one setuptools ?

TOML (the PEP standard) is preferred as it would allow support for other build systems.
Initially, when I started this PR, setuptools did not support TOML, just it's own setup.cfg, but now it does experimentally (see: pypa/setuptools#3214). I included both files at the time of writing expecting that it would be supported soon.

So we have several options:

  • Choose to keep one of the two files:
    • pyproject.toml which is a new standard only just experimentally supported in very recent versions of setuptools>=61.0.0, or
    • setup.cfg which works for only setuptools, but is considered stable and the successor to the legacy setup.py method
  • Keep both files, allowing Qiling developers to safely package and develop on setuptools, but also allows use of alternative systems like poetry or flit
  • Close this PR and maintain the legacy setup.py.

Sorry if it still doesn't make sense; Python building and packaging is a bit of a mess 😅

@wtdcode
Copy link
Member

wtdcode commented Apr 30, 2022

I still dont understand where they are two files. one is TOML and one setuptools ?

TOML (the PEP standard) is preferred as it would allow support for other build systems. Initially, when I started this PR, setuptools did not support TOML, just it's own setup.cfg, but now it does experimentally (see: pypa/setuptools#3214). I included both files at the time of writing expecting that it would be supported soon.

So we have several options:

  • Choose to keep one of the two files:

    • pyproject.toml which is a new standard only just experimentally supported in very recent versions of setuptools>=61.0.0, or
    • setup.cfg which works for only setuptools, but is considered stable and the successor to the legacy setup.py method
  • Keep both files, allowing Qiling developers to safely package and develop on setuptools, but also allows use of alternative systems like poetry or flit

  • Close this PR and maintain the legacy setup.py.

Sorry if it still doesn't make sense; Python building and packaging is a bit of a mess 😅

It's always a big mess but funny things are that everything "just" happens to work.

Personally, I prefer poetry and it looks good to me.

setup.py Outdated Show resolved Hide resolved
@nullableVoidPtr

This comment was marked as resolved.

Additionally, revert setup.py removal, remove setup.cfg and update versions
@nullableVoidPtr nullableVoidPtr changed the title Migrate to setup.cfg and PEP 518 Migrate to PEP 518 May 1, 2022
@xwings
Copy link
Member

xwings commented Oct 6, 2022

@wtdcode @kabeor @elicn there is some update and what do u guys think.

@xwings
Copy link
Member

xwings commented Oct 6, 2022

@nullableVoidPtr there is a conflict to solve :)

@kabeor
Copy link
Member

kabeor commented Oct 6, 2022

It definitely make package info more readable, I think it should be merged once the conflict is solved.

@nullableVoidPtr
Copy link
Contributor Author

Hi all,

Totally forgot about this PR, life got in the way.
@kabeor @xwings, am I still to maintain both the TOML and the old setup.py, or should Qiling use pyproject.toml only?

Update pyproject.toml to update deps and replace MANIFEST.in
@nullableVoidPtr
Copy link
Contributor Author

I've taken the liberty to remove most of the setup.py code as the TOML code is no longer experimental and should be stable.

Also, I've noticed that some variables, like version, can be marked as dynamic and depend on the contents of a file; this may be useful in conjunction with something like setuptools-scm, but I'm not sure how wanted something like that would be.

@xwings
Copy link
Member

xwings commented Oct 21, 2022

One question,

we save and read the version number from setup.py

This will be a problem if we remove from setup.py

@nullableVoidPtr
Copy link
Contributor Author

we save and read the version number from setup.py
Ah right, what with the dev branch tagging. I currently don't see any code in-tree, and it should be fairly easy to automatically edit a TOML file, but I understand if it's a part of others' workflow and if it may be difficult to change.

Okay, so one option is to have project.dynamic = ['version'], and further something like version = {attr = "setup.VERSION"}. Probably not the best way to go about it, but it might work.

If the need for the one version is not needed in setup.py, then this could be just __version__ in qiling/__init__py, removing the need for the importlib dependency there, though it's up to you.

FWIW, this diff works wonders:

diff --git a/pyproject.toml b/pyproject.toml
index 636daf17..023d426f 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -1,7 +1,7 @@
 [project]
 name = "qiling"
-version = "1.4.5-dev"
 description = "Qiling is an advanced binary emulation framework that cross-platform-architecture"
+dynamic = ["version"]
 readme = "README.md"
 requires-python = ">=3.8"
 license = {text = "GPL-2.0-only"}
@@ -69,7 +69,7 @@ changelog = "https://github.com/qilingframework/qiling/blob/master/ChangeLog"
 qltool = "qltool:main"
 
 [build-system]
-requires = ["setuptools>=61.0.0", "wheel"]
+requires = ["setuptools>=61.0.0", "setuptools_scm[toml]>=6.2", "wheel"]
 build-backend = "setuptools.build_meta"
 
 [tool.setuptools.package-data]
@@ -78,3 +78,4 @@ qiling = ["profiles/*.ql"]
 "qiling.os.uefi" = ["guids.csv"]
 "qiling.arch.evm.analysis" = ["signatures.json"]
 
+[tool.setuptools_scm]

On my machine, this builds version 1.4.5.dev279+g0dcf436c.d20221021 on my machine, but may need tweaking to fit Qiling's purposes

@xwings
Copy link
Member

xwings commented Oct 21, 2022

I guess there is a problem between qiling and qltools. that is the reason why importlib is there.

and having version number in 2 or 3 files is not a good way to do it.

@elicn what do you think ?

@elicn
Copy link
Member

elicn commented Oct 21, 2022

I like @nullableVoidPtr's approach and would happily drop the importlib thing from __init__.py.

However, I didn't understand what is the problem with Qiling / qltool.
Can't they both rely on what's in __init__.py ?

@xwings
Copy link
Member

xwings commented Oct 25, 2022

I dont remember why it all move to importlib, it was in init.py all this while until someone made some changes to it ?

maybe @wtdcode knows ?

@wtdcode
Copy link
Member

wtdcode commented Oct 25, 2022

The importlib is introduced by @nullableVoidPtr .

I moved the version to a single file and get it by manually exec because previously Qiling setup.py will assume Qiling is already installed and retrieve Qiling version by import, which violates some setup.py assumptions.

@xwings
Copy link
Member

xwings commented Oct 25, 2022

@wtdcode so, this PR is good to go ? some still need some fix ?

@wtdcode
Copy link
Member

wtdcode commented Oct 25, 2022

I will again suggest @nullableVoidPtr testing the latest dev branch on the test PyPI.

Also, I doubt if our release CI still works after this change, which should be checked on test PyPI, too. Note the release steps are skipped on non-tag commits.

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.

None yet

5 participants