-
Notifications
You must be signed in to change notification settings - Fork 516
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
Restore aca-py script for when poetry's script support is not suitable #2511
Conversation
I'm interested to hear about the scenario where the poetry script is insufficient but this seems fine to me! |
The situation we discovered with AATH failures is that when you install The ideal solution here would be to keep installing that script, I don't think this addition to the PR will do that. Or not include it and see if it becomes an issue. Using the provided/built docker images will not benefit from adding AATH fixed the missing script itself. |
My thought is that it doesn’t harm anything to be there, so let’s just leave it. It’s helpful to any installation (like AATH) that assumes it will be there. Does that make sense. @Gavinok you didn’t sign your commit for DCO. Must do for all commits. |
certainly doesn't harm to have |
Is it not just a static file sitting there? How did we do it before? If this is difficult, no need to do this… Perhaps it never was included in the distribution? |
sorry, i don't know enough (yet, i guess) about putting together pypi distributions. but AATH was excepting that shell script to be installed to |
Ah. I get it. In AATH we’re pulling it straight out of GitHub. No distribution - just a pull. |
Signed-off-by: Lucas ONeil <[email protected]> Signed-off-by: Gavin Jaeger-Freeborn <[email protected]>
Signed-off-by: Gavin Jaeger-Freeborn <[email protected]>
aef0e2c
to
2854e9b
Compare
Kudos, SonarCloud Quality Gate passed! |
When ACA-Py is pip installed with the current poetry setup, it will generate a script in the python environment that looks like this: #!/tmp/test/env/bin/python
# -*- coding: utf-8 -*-
import re
import sys
from aries_cloudagent.__main__ import script_main
if __name__ == '__main__':
sys.argv[0] = re.sub(r'(-script\.pyw|\.exe)?$', '', sys.argv[0])
sys.exit(script_main()) You can see this for yourself by going to an empty folder and running: $ python -m venv env
$ source env/bin/activate
$ pip install git+https://github.com/hyperledger/aries-cloudagent-python@main
$ cat env/bin/aca-py You can do the same with a cloned project: $ git clone [email protected]:hyperledger/aries-cloudagent-python.git
$ cd aries-cloudagent-python
$ python -m venv env
$ source env/bin/activate
$ pip install -e .
$ cat env/bin/aca-py So I'm a bit confused. AATH can (probably should?) install the ACA-Py into its environment such that the script will be present. |
In case it's relevant, this was using pip version |
Thanks, @dbluhm — that was not clear to me. AATH is, I think doing the first example — pip install of the github repo Let’s just cancel this PR and I’ll do a little digging in AATH. We have a working fix for now, so not the end of the world. I had just thought that the bin/aca-py was just removed because poetry provided a different mechanism, and figured just adding the file back in would help others like AATH were expecting it — a trivial redundent change, not needed but harmless. |
oh boy, i couldn't help myself... something truly hinky going on. last week, the docker image (AATH) couldn't build because it could not find the this morning, i just fired up a docker python image (python:3.9-slim-bullseye and added git/pip) then did pip install for various commits that failed last week (including main) and now they all have the i am perplexed. we have failures in GHA and failures locally now (at least locally) those can't be reproduced. Was this a hiccup in TLDR - close this PR as @swcurran suggested. Apparently this was never broken. 🤷 |
Looking at this a bit because I had a chance to… @usingtechnology — the
The
Is the problem that AATH made assumptions about the shell script? I’m not quite sure what that regex in the new script does... |
the top script is an exact copy of the bin/aca-py script that used to get included in the distribution (and was in our repo). the second is what the poetry distribution installs/creates. the problem was: for N days, the poetry distribution was not creating that script on install. no idea why it wasn't. after I added the fix to AATH (just adding the script in "manually" to AATH image) the installs started working... i even went back and built/ran AATH using a bunch of the "bad" commit hashes that I documented and they all worked. Kind of frustrating really. We could remove the "fix" from AATH. |
This should resolve some edge cases where the poetry script
aca-py
did not fully replace the need for the dedicated script file inbin/aca-py
mentioned in #2436.