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

dist/tools/bmp: in-line dependencies using PEP 723 #21131

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

basilfx
Copy link
Member

@basilfx basilfx commented Jan 12, 2025

Contribution description

PEP 723 is an accepted proposal to in-line script metadata. One such use cases is to list script dependencies inside the script, which makes the requirements.txt redundant.

With the correct shebang, it becomes even possible to run the script directly, without creating a virtual environment and installing dependencies in advance. Combining both makes the execution of single-file scripts within RIOT much easier and friendlier.

PEP 723 metadata can be used by pipx and can be easily installed when pip is available. It is maintained by the PyPA, just like pip.

I chose this file to start with, but there are at least five more scripts with requirements.txt that could benefit from the same change. Because pip install still requires a requirements.txt file, which is generally accepted during development, I kept both for now. Aan open issue exists to add support for reading dependencies from PEP 723, which might make it superfluous in the future.

Testing procedure

Ensure that pipx is installed.

Alternatively, execute it from the dist/tools/bmp folder.

This script is part of the Black Magic Debugger. Execute it from the dist/tools/bmp folder, or compile any application and use PROGRAMMER=bmp to execute it from the build process.

Issues/PRs references

None

@github-actions github-actions bot added Area: doc Area: Documentation Area: tools Area: Supplementary tools labels Jan 12, 2025
@mguetschow
Copy link
Contributor

While I like the idea of getting rid of separate requirements.txt files and legacy pip, it's a bit unfortunate that the dependency information is now doubled in both files. I think you should add least add a comment on both sites that they need to be kept in sync.

@benpicco benpicco requested review from miri64 and maribu January 13, 2025 18:42
@maribu
Copy link
Member

maribu commented Jan 23, 2025

I think we can just drop the requirements.txt. Couldn't we?

@basilfx
Copy link
Member Author

basilfx commented Feb 2, 2025

Yes, the requirements.txt file could be deleted, but at a (IMHO minor) cost of making development a bit more difficult.

As a Python developer, I would be fine with it.

@maribu
Copy link
Member

maribu commented Feb 2, 2025

OK, if t here is an actual benefit of keeping the reuiqrements.txt, we can also just keep it.

@maribu maribu enabled auto-merge February 2, 2025 22:30
PEP 723 is an accepted proposal to in-line script metadata. One such
use cases is to put script dependencies inside the script, which makes
the requirements.txt redundant [1].

With the correct shebang, it becomes even possible to run the script
directly, without creating a virtual environment and installing
dependencies in advance.

Combining both makes the execution of single-file scripts within RIOT
much easier and friendlier.

PEP 723 metadata can be used by `pipx` [2] and can be easily
installed when `pip` is available.

[1] `pip install` still requires a requirements.txt, which is
    generally accepted during development. An open issue exists to
    add support for reading dependencies from PEP 723 as well. See
    pypa/pip#12891
[2] https://github.com/pypa/pipx
@basilfx
Copy link
Member Author

basilfx commented Feb 4, 2025

@mguetschow I have added a comment to the bmp.py file as well.

@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 4, 2025
@riot-ci
Copy link

riot-ci commented Feb 4, 2025

Murdock results

✔️ PASSED

652285a fixup! dist/tools/bmp: in-line dependencies using PEP 723

Success Failures Total Runtime
1 0 1 01m:25s

Artifacts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: doc Area: Documentation Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants