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

Baseimage 0.0.4 #1705

Merged
merged 1 commit into from
Nov 25, 2021
Merged

Baseimage 0.0.4 #1705

merged 1 commit into from
Nov 25, 2021

Conversation

pSchlarb
Copy link
Member

Baseimage changes as discussed in #1684

@pSchlarb pSchlarb requested a review from a team as a code owner October 28, 2021 08:41
@sovbot
Copy link

sovbot commented Oct 28, 2021

Can one of the admins verify this patch?

@pSchlarb pSchlarb force-pushed the baseimage-0.0.4 branch 2 times, most recently from 0e1260c to 12bf74a Compare October 28, 2021 09:13
Copy link
Contributor

@udosson udosson left a comment

Choose a reason for hiding this comment

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

@pSchlarb thank you very much for this PR.
This PR should only contain the updated Docker files and the shouldn't be referenced anywhere in the PR.
The workflow is the following.

  • Update Docker files (First PR - this PR)
  • Merge into master
  • Build new Docker Images
  • Push to DockerHub
  • Update Build pipelines with new Docker image (0.0.4) (Second PR)
  • Merge into master

.github/workflows/build/Dockerfile Outdated Show resolved Hide resolved
.github/workflows/lint/Dockerfile Outdated Show resolved Hide resolved
acceptance/indy-cli-batches/AS-03-01-steward.batch Outdated Show resolved Hide resolved
'pip<10.0.0' \
setuptools \
RUN pip3 install \
cython\
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do install cython? This can be removed
Doesn't pip needs to be installed (without pinned to a specific version)?

Copy link
Member Author

Choose a reason for hiding this comment

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

If it is not installed the pip install .[tests] complains about distutils.errors.DistutilsError: Could not find suitable distribution for Requirement.parse('Cython>=0.20')

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you replace your pip3 install with the following, please?

RUN pip3 install -U \
    pip==20.3.4 \
    virtualenv==20.0.35 \
    "setuptools<=50.3.2"

@pSchlarb
Copy link
Member Author

pSchlarb commented Nov 4, 2021

Will do and test the cython and linking part.

@pSchlarb
Copy link
Member Author

pSchlarb commented Nov 4, 2021

@udosson I will update #1684 after this PR is merged, if its okay?

Copy link
Contributor

@udosson udosson left a comment

Choose a reason for hiding this comment

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

@pSchlarb thanks for applying the requested changes. There is only one thing to do.
When building your image, pip is at version 8.1.1.

$ pip list
pip (8.1.1)

I recommend installing the latest supported version of pip which is at 20.0.35.
cython shouldn't be needed then.
Could you very please if pip install .[tests] works with the new image on your PR #1684?

'pip<10.0.0' \
setuptools \
RUN pip3 install \
cython\
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you replace your pip3 install with the following, please?

RUN pip3 install -U \
    pip==20.3.4 \
    virtualenv==20.0.35 \
    "setuptools<=50.3.2"

@pSchlarb
Copy link
Member Author

pSchlarb commented Nov 5, 2021

With that newer pip version you have to use --use-deprecated legacy-resolver or pip will complain about not finding the correct plenum packages:

  WARNING: Requested python3-indy==1.15.0-dev-1625 from https://files.pythonhosted.org/packages/a0/78/74c1d6206c1eae93f677abd6ff03bd6357412716615f0df6858bd655b6a1/python3-indy-1.15.0-dev-1625.tar.gz#sha256=821729e796a47fd591520c8c73fefc91dce7bfc9f784c6702ca9d03d82f2e76a (from indy-node==1.13.0.dev0), but installing version 1.15.0
WARNING: Discarding https://files.pythonhosted.org/packages/a0/78/74c1d6206c1eae93f677abd6ff03bd6357412716615f0df6858bd655b6a1/python3-indy-1.15.0-dev-1625.tar.gz#sha256=821729e796a47fd591520c8c73fefc91dce7bfc9f784c6702ca9d03d82f2e76a (from https://pypi.org/simple/python3-indy/). Requested python3-indy==1.15.0-dev-1625 from https://files.pythonhosted.org/packages/a0/78/74c1d6206c1eae93f677abd6ff03bd6357412716615f0df6858bd655b6a1/python3-indy-1.15.0-dev-1625.tar.gz#sha256=821729e796a47fd591520c8c73fefc91dce7bfc9f784c6702ca9d03d82f2e76a (from indy-node==1.13.0.dev0) has different version in metadata: '1.15.0'
ERROR: Could not find a version that satisfies the requirement python3-indy==1.15.0-dev-1625 (from indy-node[tests])
ERROR: No matching distribution found for python3-indy==1.15.0-dev-1625

And thereafter complains about zipp, which seems to be a dependency of importlib-metadata>=2.0 comming from plenum:

Requirement already satisfied: zipp>=0.5 in /usr/local/lib/python3.5/dist-packages (from importlib-metadata>=2.0->indy-plenum==1.13.0.dev143->indy-node==1.13.0.dev0) (3.6.0)
ERROR: Package 'zipp' requires a different Python: 3.5.2 not in '>=3.6'

@pSchlarb
Copy link
Member Author

pSchlarb commented Nov 5, 2021

@udosson Should zipp be therefore pinned in the baseimages, since the requirement for it is only >=0.5?

@udosson
Copy link
Contributor

udosson commented Nov 5, 2021

How could we solve the issue with the newer version of pip without using --use-deprecated legacy-resolver? Do you know the root of the issue?

zipp
The error message complains about a version of zipp that has no support for Python 3.5. I pinned importlib-metadatato v2.1.1in this commit (hyperledger/indy-plenum@5bef312)
You could therefore use another version of Indy-Plenum in setup.py which should resolve the issue. Namely the PyPI packages that gets published in this gha run https://github.com/hyperledger/indy-plenum/actions/runs/1418274109

@pSchlarb
Copy link
Member Author

pSchlarb commented Nov 5, 2021

To the issue of the deprecated legacy-resolver:
In the old versions of pip it doesn't check the versions of packages strictly.
And the versions mismatch with the metadata provided. (If i recall it correctly in the metadata gotten from pypi only is something like 1.15.0is set but the setup.py requires python3-indy==1.15.0-dev-1625)
I will check where exactly the issue is.

@pSchlarb
Copy link
Member Author

pSchlarb commented Nov 5, 2021

I think i found the issue with python-indy and deprecated resolvers:
In https://github.com/hyperledger/indy-sdk/blob/master/wrappers/python/setup.py line 4:
PKG_VERSION = os.environ.get('PACKAGE_VERSION') or '1.15.0'
the version is statically set to 1.15.0 if the PACKAGE_VERSION environmentvariable doesn't exist.

@udosson
Copy link
Contributor

udosson commented Nov 24, 2021

@pSchlarb thanks for adjusting the PR. I found the following in my notes:

# pypi based packages
RUN pip3 install -U \ 
    'pip<10.0.0' \
    'setuptools<=50.3.2' 

# needs to be installed separately and pinned to version 20.0.25 to be compatible with Python3.5 and packages like zipp==1.2.0
RUN pip3 install -U \
    'virtualenv==20.0.35'

Could you please, follow the steps from Jenkins CI (https://github.com/hyperledger/indy-node/blob/master/Jenkinsfile.ci#L74) until line 87 locally and verify that virtuelenv is working and that zipp is installed at version 1.2.0?

Once you've tested that and everything is working, could you squash merge your commits into one, please?
Then I'll merge your PR.

Signed-off-by: Philipp Schlarb <p.schlarb@esatus.com>
@pSchlarb
Copy link
Member Author

Yes zipp is installed and the venv is working.
And the tests are running properly.

@udosson
Copy link
Contributor

udosson commented Nov 24, 2021

(ci) test this please

Copy link
Contributor

@udosson udosson left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @pSchlarb

@udosson udosson merged commit 305adee into hyperledger:master Nov 25, 2021
@pSchlarb pSchlarb deleted the baseimage-0.0.4 branch November 25, 2021 17:16
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.

None yet

3 participants