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

Restore aca-py script for when poetry's script support is not suitable #2511

Closed
wants to merge 2 commits into from

Conversation

Gavinok
Copy link
Contributor

@Gavinok Gavinok commented Sep 22, 2023

This should resolve some edge cases where the poetry script aca-py did not fully replace the need for the dedicated script file in bin/aca-py mentioned in #2436.

@dbluhm
Copy link
Member

dbluhm commented Sep 22, 2023

I'm interested to hear about the scenario where the poetry script is insufficient but this seems fine to me!

@usingtechnology
Copy link
Contributor

The situation we discovered with AATH failures is that when you install aries_cloudagent as a dependency the bin/aca-py shell script no longer gets installed. AATH backchannels specifically launch instances by calling bin/aca-py start ....

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 bin\aca-py back in as they work using the poetry command.

AATH fixed the missing script itself.

@swcurran
Copy link
Member

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.

@usingtechnology
Copy link
Contributor

certainly doesn't harm to have bin/aca-py added to the distribution as before. will have to research how to include that in the distribution and have the file added to the filesystem like before. i don't know the details of the pip install or the poetry install and how to configure the pyproject.toml to act like the setup.cfg/setup.py from previous distributions.

@swcurran
Copy link
Member

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?

@usingtechnology
Copy link
Contributor

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 /usr/local/bin. That wasn't there once we hit a poetry distribution. Ideally when you do pip install or poetry install we would find the file there and no on would have to adapt their existing code.

@swcurran
Copy link
Member

Ah. I get it. In AATH we’re pulling it straight out of GitHub. No distribution - just a pull.

loneil and others added 2 commits September 24, 2023 14:02
Signed-off-by: Lucas ONeil <lucasoneil@gmail.com>
Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
Signed-off-by: Gavin Jaeger-Freeborn <gavinfreeborn@gmail.com>
@sonarcloud
Copy link

sonarcloud bot commented Sep 24, 2023

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

@dbluhm
Copy link
Member

dbluhm commented Sep 25, 2023

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 git@github.com: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.

@dbluhm
Copy link
Member

dbluhm commented Sep 25, 2023

In case it's relevant, this was using pip version 22.3.1

@swcurran
Copy link
Member

Thanks, @dbluhm — that was not clear to me.

AATH is, I think doing the first example — pip install of the github repo @main. It’s not using venv. For whatever reason, it is not finding the aca-py script where it used to be, hence the error and the adjustment made in AATH.

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.

@usingtechnology
Copy link
Contributor

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 /usr/local/bin/aca-py file to symlink and executeRUN ./bin/aca-py --version > ./acapy-version.txt... Which is how I ended up just putting the file in myself and everything worked.

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 /usr/local/bin/aca-py file.

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 pip/pypi? Was it not completing the installation?

TLDR - close this PR as @swcurran suggested. Apparently this was never broken. 🤷

@swcurran
Copy link
Member

Looking at this a bit because I had a chance to…

@usingtechnology — the aca-py script you are using is this:

#!/bin/sh

if [ -z "$PYTHON" ]; then
    PYTHON=`command -v python3`
    if [ -z "$PYTHON" ]; then
        PYTHON=python
    fi
fi

$PYTHON -m aries_cloudagent "$@"

The /usr/local/bin/aca-py is this:

#!/usr/local/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())

Is the problem that AATH made assumptions about the shell script? I’m not quite sure what that regex in the new script does...

@usingtechnology
Copy link
Contributor

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.

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.

5 participants