-
Notifications
You must be signed in to change notification settings - Fork 5
feat: requirements.txt support for maven plugin #30
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
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
|
Hello, thank you for the contribution! It seems that the code is somehow reformatted, which breaks the diff and makes it difficult to review. Can you please fix that?
Can you explain how it's going to interact with the Maven based configuration? I assume it is going to be one or the other? Can you add some documentation into the PR description that we could then put to https://www.graalvm.org/python/docs/#embedding-build-tools, or even better make a companion PR to update the docs: https://github.com/oracle/graalpython/blob/master/docs/user/Embedding-Build-Tools.md? We need some tests. See other tests in We also require contributors to sign contributor agreement. It is a common practice, see the comment from the bot that checks this for more details. |
|
Thank you very much for the review and for pointing this out! You’re right, the code was accidentally reformatted — I’ll revert the unrelated formatting changes and update the PR. For the configuration, the idea is to have a single source of Python dependencies: either the block in the Maven configuration or a requirements.txt file. If both are present, the build will fail with a clear error. I went with requirements.txt to align with common Python tooling conventions and make it easier to reuse existing dependency files. I’ll update the PR description, add a brief note to the documentation you referenced, and incorporate integration tests to cover this behavior. |
f42c7af to
9fee83f
Compare
9fee83f to
cae4f22
Compare
|
It looks like you may have accidentally removed some existing code from the integration tests. Also, please use only English in the code, including comments. Otherwise, looks like it's on a good trajectory! |
|
Also please note that signing the Oracle contributor agreement is necessary requirement before we can merge this PR. |
|
Thanks for pointing this out! |
cae4f22 to
9d151f5
Compare
|
Thank you, this looks good. I see that We planned similar feature, but wanted to make it such that if you specify |
|
Thanks for the feedback! In my current implementation, I parse the I can update the PR to match this approach. |
886c8d7 to
b224c9b
Compare
|
I've updated the logic to support requirements.txt and align with pip’s native behavior.
The implementation clearly separates two independent flows:
This prevents configuration mixing and keeps the behavior explicit. |
b224c9b to
9df0778
Compare
Description
Added support for reading Python dependencies from requirements.txt instead of standard tags in Maven configuration.
Fixes # (issue)
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
requirements.txtpresentrequirements.txtis absentTest Configuration:
Checklist: