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

#2289 Migrate to Poetry #2436

Merged
merged 27 commits into from
Aug 30, 2023
Merged

Conversation

Gavinok
Copy link
Contributor

@Gavinok Gavinok commented Aug 19, 2023

This PR resolves #2289

I migrated the use of requirements.txt and setup.py over to poetry. The use of multiple requirements.*.txt is replaced with dependency groups in the pyproject.toml. I have also added a dedicated [tool.poetry.group.test.dependencies] as opposed to the original requirements.dev.txt which contained both developer dependencies and testing dependencies.

@Gavinok Gavinok marked this pull request as draft August 19, 2023 00:33
@dbluhm
Copy link
Contributor

dbluhm commented Aug 21, 2023

I believe we want to install the dependencies for askar, bbs, and indy (though this one should go away very soon) as extras rather than as groups. From the poetry documentation:

Dependency groups, other than the implicit main group, must only contain dependencies you need in your development process. Installing them is only possible by using Poetry.

To declare a set of dependencies, which add additional functionality to the project during runtime, use extras instead. Extras can be installed by the end user using pip.

@Gavinok Gavinok force-pushed the migrate-to-poetry branch from ef48b81 to d1baab1 Compare August 22, 2023 17:29
@Gavinok Gavinok marked this pull request as ready for review August 22, 2023 18:16
@Gavinok Gavinok marked this pull request as draft August 22, 2023 18:17
pyproject.toml Outdated Show resolved Hide resolved
@Gavinok Gavinok assigned Gavinok and unassigned Gavinok Aug 22, 2023
@Gavinok Gavinok marked this pull request as ready for review August 22, 2023 22:25
bin/aca_py.py Outdated Show resolved Hide resolved
bin/aca_py.py Outdated Show resolved Hide resolved
@Gavinok Gavinok marked this pull request as draft August 23, 2023 20:11
@Gavinok Gavinok marked this pull request as ready for review August 24, 2023 19:49
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@usingtechnology usingtechnology left a comment

Choose a reason for hiding this comment

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

Works great - awesome job.

Can we add a Poetry.md file in the root of the project that provides a cheat sheet for developers that have not used poetry before? Particularly around virtual envs and usage for running tasks through poetry or not using virtual envs. Also, should explain where and how poetry is used to build and publish the library.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@usingtechnology
Copy link
Contributor

pinging @dbluhm for a final approval.

@swcurran
Copy link
Contributor

@Gavinok -- a question for you about this PR. We discovered that the problem with the Aries Agent Test Harness that was triggered by this PR was the removal of the "bin/aca-py" shell script. From scanning the commits, it looks like that was done deliberately, but I wanted to confirm that, and the rationale for the change. It is a breaking change we'll need to document in the changelog in case anyone (like AATH) is using the file. Do you think it would be a bad thing to put the file back or in changing to poetry should it be removed?

I see these two commits where the change was made:

Thanks!

@Gavinok
Copy link
Contributor Author

Gavinok commented Sep 22, 2023

@Gavinok -- a question for you about this PR. We discovered that the problem with the Aries Agent Test Harness that was triggered by this PR was the removal of the "bin/aca-py" shell script. From scanning the commits, it looks like that was done deliberately, but I wanted to confirm that, and the rationale for the change. It is a breaking change we'll need to document in the changelog in case anyone (like AATH) is using the file. Do you think it would be a bad thing to put the file back or in changing to poetry should it be removed?

I see these two commits where the change was made:

Thanks!

This may be a misunderstanding on my part of what poetry did with its scripts functionality. We should be able to add the original script back into the repository. The idea was that the [tool.poetry.scripts] entry in the pyproject.toml would replace it once installed.

@swcurran
Copy link
Contributor

OK — could you do a quick and dirty PR to add it back? If it is safe enough, let’s just put it back and not have to explain it to someone. It was tougher than it should have been to find in AATH.

@Gavinok
Copy link
Contributor Author

Gavinok commented Sep 22, 2023

Sure thing, I can't say with 100% confidence that it will work in all cases but clearly the current solution isn't helping filling it's place

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.

Transition ACA-Py to use poetry
4 participants