Bug 2181686

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.