ci: Add FedoraProject rpm spec file

[LecrisUT]

This will make it easier for maintainers to keep up to date with upstream package.

Note the version numbering is currently hard coded. It might be possible to retrieve it dynamically, but I'm not sure if it will work on copr. However this should not be much of an issue as long as we can get some notifications of the builds.

[LecrisUT]

In order to integrate with copr there are 2 ways, either we use my current copr repo setup, I give persmissions to the maintainers to manage it and we switch the hook to be called by this repo. Or one of the maintainers can create a copr repo (I can walk you through it if needed), and setup a webhook there. It is also possible to create groups, but this needs to be requested on the Fedora channels.

We can limit the webhook to tag creation, labels, etc. so it only triggers a build on new releases. In principle it should work, but I haven't tested this.

After that it is trivial for the maintainer to set a notification either on github or directly on copr. Need to check if they need builder permission to get notification on other's package for the latter.

[henryiii]

Is there an easy way I can try this locally (and therefore in Actions)?

FYI, rpmlint gives: scikit-build-core.spec:6: W: mixed-use-of-spaces-and-tabs (spaces: line 6, tab: line 4)

[LecrisUT]

The running on Actions is a bit tricky because in my experience with mock is that it gets a bit fussy when built locally with ForgeMeta (names of the tar ball created and expected name from the spec mismatch). I need to tinker a lot to find an actual workflow that works with this setup. Copr seems to use rpkg but I didn't look into how it manages to work.

Realistically though, even without local testing with actions, it would be a great help, especially since these builds should only be checked at release times.

[LecrisUT]

Ok, thanks to the folks from FedoraProject, I've figured tito is the best. Locally we can now build using:

$ tito build --srpm --test --output=srpm_dir
$ mock srpm_dir/name_of_srpm.src.rpm

Note that tito does not pick up uncommitted changes.

[LecrisUT]

We seem to need this upstream PR to be able to build using tito and vcs

[LecrisUT]

Ok, I got the spec to work, but it is not ideal. I think upstream setuptools_scm has a bug:
https://github.com/pypa/setuptools_scm/blob/f620057203f1446c1dea2ce57f73dccdd6f28ee9/src/setuptools_scm/git.py#L246-L250
That code errors on my side if it not the form vXYZ-0-ABC, so the if len(split) < 3 is never triggered.

Could you confirm that with the .git_archival.txt from the tar ball the project is buildable? Anyway could you check the errors in the most recent builds? This one suggests that both cmake and ninja-build are required for running. The former is acceptable, but why can't we use a different build system for the latter?

[henryiii]

What form is it to cause this? The thing I see from looking at that is it's possibly missing the leading "v" stripping?

I can check the tarball.

We probably require cmake and ninja to run the tests. You don't need ninja to use scikit-build, make works fine too, it just the tests that likely require it. We probably can make the ninja-specific parts get skipped if ninja's not installed, but that probably hasn't been done, since GHA has ninja.

[LecrisUT]

So I have tested with both v0.2.0 and v0.2.0-1.git... It seems to require at least 2 dashes to be correctly picked up.

I have double-checked and redid the tests with Requires: cmake, but that didn't help. Seems to be a different issue. I misread the no module cmake are skipped so that was a red-hearing. Don't know what the real error is about though.

[henryiii]

>>> "v0.2.0".rsplit("-", 2)
['v0.2.0']

Which has a length < 3, so that seems like it would pass. I'm guessing the leading v is stripped earlier.

[LecrisUT]

Oh, after a bit more debugging this was fixed in pypa/setuptools-scm@4c2cf6e.

But that was introduced in setuptools_scm v7.1.0, and the Fedora 37 that I work on by default has v7.0.5. Sorry for the confusion on that.

Well the current version temporarily fixes it for F37, but it lacks other dependencies, so I think I'll drop it anyway.

There are still the build issues like

E  pytest_subprocess.exceptions.ProcessNotRegisteredError: The process '/usr/bin/cmake3 --version' was not registered.

Not sure how to debug these though, any ideas? cmake3 works just fine in the mock environment:

$ cmake3 --version
cmake version 3.26.0-rc4

CMake suite maintained and supported by Kitware (kitware.com/cmake).

[LecrisUT]

Weird. The last error, when I manually run the tests, it doesn't fail, but for some reason it reliably fails when I run initially. Any insights around test_pep517_sdist_hash?

https://download.copr.fedorainfracloud.org/results/lecris/scikit-build-core/fedora-38-x86_64/05574434-scikit-build-core/builder-live.log.gz

[henryiii]

Hmm, that's not a critical test, but it shouldn't be failing. That's checking that the SDist is binary reproducible. And you only have one of the tests failing. I'll see if I can come up with a way to detect what is different between the two files...

[henryiii]

What's the difference between "manually run the tests" and "run initially"?

[LecrisUT]

That's what puzzles me. Afaik there shouldn't be anything that affects it. If we can extract them, say maybe by binary dumping the files and reconstructing from that, maybe we can find out.

Any idea on how to locally edit and get that?

[LecrisUT]

I have dumped the file as a base64 and tried to recreate it according to the latest temporary head commit.

test.tar.gz

The only difference I notice is that the dates are different

[LecrisUT]

@henryiii so what do you think? Should this issue be fixed in the test, or should we disable this test and incorporate this as is?

This issue on git, and this one on tito are related, but not necessary for this PR.

[henryiii]

Dates should be locked when making SDists (at least if the reproducible setting is on, which it is for this test). This isn't a very important test, but it would be nice to solve.

[henryiii]

Is SOURCE_DATE_EPOCH set, perhaps? That would break this test, I think.

[henryiii]

Yeah, that's it, I see in the logs: setting SOURCE_DATE_EPOCH=1677456000. We can unset that for the tests that care about it.

[LecrisUT]

I was looking at the installed files. Can we remove resources/find_python since it is guaranteed to be present by the cmake dependency? Don't need to edit the installation or patch anything for that right?

[henryiii]

Well, it will likely be patched before 3.26.1 comes out: 32f38e7

In general, as long as the CMake version is new enough, this won’t be used, but I don’t think there’s harm in keeping it. (And “new enough” will be the unreleased 3.26.1 very soon due to this upstream bug).

[LecrisUT]

I was thinking about the discussion that was had on scikit-build about bundling files, and if that issue can arise again here. In principle I think it should be fine since we are not conflicting paths and searches, it is only used internally, but would be nice to keep track of such changes and clean up the repo every now and then. Still waiting on review comments so maybe it's a non-issue altogether.

[codecov]

Codecov Report

Merging #201 (08d52be) into main (5bce84a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #201   +/-   ##
=======================================
  Coverage   89.35%   89.35%           
=======================================
  Files          50       50           
  Lines        2160     2160           
=======================================
  Hits         1930     1930           
  Misses        230      230           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[LecrisUT]

Ok, this PR should be ready for review now. Anything that we want to add to this? Otherwise, just adding a webhook after it is merged would be the last thing to do

[henryiii]

I'd still like some sort of smoke check, even if it's just a static analysis one that checks to see if the dependencies haven't changed and if the version is correct. I'm not going to remember to update the version - this is the only instance with a hardcoded version in the repo at the moment.

But not a critical blocker if you want it in now.

[LecrisUT]

Oh, it picked up some errors after rebasing onto main: https://download.copr.fedorainfracloud.org/results/lecris/scikit-build-core/fedora-38-aarch64/05686616-scikit-build-core/builder-live.log.gz

[LecrisUT]

I'd still like some sort of smoke check, even if it's just a static analysis one that checks to see if the dependencies haven't changed and if the version is correct. I'm not going to remember to update the version - this is the only instance with a hardcoded version in the repo at the moment.

But not a critical blocker if you want it in now.

I should be able to do that, but it would be a bit tricky to maintain because I haven't figured how to make dynamic version in the spec file. Currently we are running a workaround because of a missing feature in tito/git archive:

if grep -q "describe-name: \$Format" .git_archival.txt; then
   sed -i "s/describe-name:.*/describe-name: v%{version}/g" .git_archival.txt
fi

For upstream there is a different repo: https://src.fedoraproject.org/rpms/python-scikit-build-core/, because that one has to be built with fixed tar balls, while this one we want it to be built against latest version. This one should only be used for testing, i.e. a CI for fedora maintenance, so maybe having version difference will not be that critical. I'll still try to figure out how to make the version dynamic with fedora folks

[henryiii]

Are you using cmake 3.26.0? I think there's a bug if you have 3.26.0 exactly where it wasn't using the backported fix from the upcoming 3.26.1 - fixed in #238 (ahh, that's not even merged yet, that's probably why).

[LecrisUT]

Yep, that fixed it. All green

[henryiii]

(In theory, I think you can merge this)

[LecrisUT]

(In theory, I think you can merge this)

Apparently needs more permission to write to protected branch

[henryiii]

Now?

[LecrisUT]

I should have asked, but what naming convention to use for PRs and/or commits?

[henryiii]

https://www.conventionalcommits.org/en/v1.0.0/ if possible.