Spec URL: https://djw8605.fedorapeople.org/python-scitokens.spec SRPM URL: https://djw8605.fedorapeople.org/python-scitokens-1.7.4-1.el8.src.rpm Description: The SciTokens library is a creation and parsing library for capability JWT's, with its own enforcing and scope language. It is used by tools such as HTCondor. The linked srpm was successfully built with a scratch build using fedora's koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=99100559 fedpkg lint reports no issues. Fedora Account System Username: djw8605
Copr build: https://copr.fedorainfracloud.org/coprs/build/5706985 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2181686-python-scitokens/fedora-rawhide-x86_64/05706985-python-scitokens/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Hi Derek, Comments reading through the .spec file and source: 1. Requires: python3-jwt >= 1.6.1 Requires: python3-cryptography These are not needed as computed anyway. rpm -qp --requires /var/lib/mock/fedora-rawhide-x86_64/result/python3-scitokens-1.7.4-1.fc39.noarch.rpm | grep 3. python(abi) = 3.11 python3.11dist(cryptography) python3.11dist(pyjwt) >= 1.6.1 python3.11dist(setuptools) which raises the point that I expect that setuptools is not needed runtime so try and get rid of that is possible. 2. export PYTHONPATH="%{buildroot}%{python3_sitelib}" (cd tests/ && %{__python3} -m pytest --verbose -ra .) Can the %pytest macros be used for that ? It does something like that. 3. src/scitokens/urltools.py seems to MIT license and is a bundling of https://github.com/itzik-h/urltools Can this be unbundled and a new package for urltools be created? And some more optional stuff: 1. Package is using the old macros - https://docs.fedoraproject.org/en-US/packaging-guidelines/Python_201x/ , Maybe switch to the new pyproject macros - https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/ Choice is free however. 2. Use %pypi_source macro for source URL?
From the review template the only significant thing is: Source checksums ---------------- https://files.pythonhosted.org/packages/source/s/scitokens/scitokens-1.7.4.tar.gz : CHECKSUM(SHA256) this package : 879598322354cdf436d41824304993c6c9c55859d9cdb6d43530cd879e843a8c CHECKSUM(SHA256) upstream package : d215091b5a66d8cf37d386602495e93f347646f132469091ca44148683193c4e diff -r also reports differences
To help things along: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=2181994 is a package review for urltools.
(In reply to Steve Traylen from comment #2) > > Comments reading through the .spec file and source: > > 1. Requires: python3-jwt >= 1.6.1 > Requires: python3-cryptography > > These are not needed as computed anyway. > Done > > which raises the point that I expect that setuptools is not needed runtime > so try and get rid of that is possible. I'll look into it, but I think you are right, setuptools isn't needed. Since I am the upstream, I can make these changes. > > 2. export PYTHONPATH="%{buildroot}%{python3_sitelib}" > (cd tests/ && %{__python3} -m pytest --verbose -ra .) > > Can the %pytest macros be used for that ? It does something like that. Done, good suggestion. > > 3. src/scitokens/urltools.py seems to MIT license and is a bundling of > https://github.com/itzik-h/urltools Can this be unbundled and a new package > for urltools be created? > I see you have submitted this package. Thank you very much! > > And some more optional stuff: > > 1. Package is using the old macros - The text in the newer guidelines implied that I should use the older macros if targeting EPEL8, which I certainly am. > > 2. Use %pypi_source macro for source URL? Done. Some of these changes require a new release of scitokens, which takes a bit more time to produce. How do you want to handle these changes, especially the urltools change. Can we move forward here while we will change scitokens to use the urltools package when it is accepted into EPEL?
I think it's fine to reference an upstream patch for the to be released scitokens. I may have a comment for the urltools package but I'll make it there.
I propose we remove the dependency on python-urltools. SciTokens uses only 1 function (~25 lines) unmodified from the abandoned python-urltools github repo. The other function is modified to suit the needs of scitokens. There certainly is appropriate attribution. And since the urltools code was copyrighted in 2014, and it hasn't been put into fedora already, it's unlikely to be reused elsewhere in fedora. I'm open to ideas.
Sounds good to me.
Ok, what's the next steps on this? Do I need to make a new srpm with the changes above?
Derek, it looks like you removed the urltools dependency from https://github.com/scitokens/scitokens/blob/master/configs/python-scitokens.spec. Should we cut a new version of SciTokens for review?
New version of python-scitokens addressing all comments is available: https://djw8605.fedorapeople.org/python-scitokens-1.8.0-1.fc38.src.rpm https://djw8605.fedorapeople.org/python-scitokens.spec I think we're good to go here.
Created attachment 1982468 [details] The .spec file difference from Copr build 5706985 to 6259190
Copr build: https://copr.fedorainfracloud.org/coprs/build/6259190 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2181686-python-scitokens/fedora-rawhide-x86_64/06259190-python-scitokens/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
The build failed due to a set of tests requiring external network connections. I can disable those tests in the spec file, is that the best practice here?
(In reply to Derek from comment #14) > The build failed due to a set of tests requiring external network > connections. I can disable those tests in the spec file, is that the best > practice here? Yes disabling those particular tests is exact correct thing to do.
Fixed the tests and scratch build worked. SRPM: https://djw8605.fedorapeople.org/python-scitokens-1.8.1-1.fc38.src.rpm SPEC: https://djw8605.fedorapeople.org/python-scitokens.spec Scratch Build: https://koji.fedoraproject.org/koji/taskinfo?taskID=104590184 Now I think we're good.
Created attachment 1982571 [details] The .spec file difference from Copr build 6259190 to 6260692
Copr build: https://copr.fedorainfracloud.org/coprs/build/6260692 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2181686-python-scitokens/fedora-rawhide-x86_64/06260692-python-scitokens/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Steve, are we good to move forward with getting this package into EPEL?
Hi Steve, Is there anything else that needs to be addressed in the packaging? If it's all ok, do you have a rough ETA for how long it would take to get into EPEL? Thanks, Brian
APPROVED
The Pagure repository was created at https://src.fedoraproject.org/rpms/python-scitokens
Looks like you released this.