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

propose-upstream does not adjust the spec's Version: #1515

Closed
martinpitt opened this issue May 23, 2022 · 11 comments · Fixed by cockpit-project/cockpit#17450
Closed

propose-upstream does not adjust the spec's Version: #1515

martinpitt opened this issue May 23, 2022 · 11 comments · Fixed by cockpit-project/cockpit#17450
Labels
area/user-experience Usability issue

Comments

@martinpitt
Copy link

martinpitt commented May 23, 2022

We tried to use packit for Fedora with cockpit, using this packit.yaml. Aside from the already filed hacks (#1505 and packit/packit#1560), we have this hack:

   post-upstream-clone:
    # packit will overwrite the version in its "fix spec file" stage
    - tools/create-spec --version 0 --build-all -o cockpit.spec tools/cockpit.spec.in

We need this because our git repo does not contain a cockpit.spec, it needs to be built from a template. This works fine for COPR builds, but not for propose_downstream -- the release failed like this:

WARNING Version '0' in spec file is outdated
[...]
DEBUG  Upstream archive name: cockpit-0.tar.xz
DEBUG  Archive 'cockpit-0.tar.xz' not found in the lookaside cache.

So apparently that "fix spec file" magic does not get applied to propose_downstream stage?

Ideally packit would take cockpit.spec from the downloaded release tarball -- everything is correct there. I tried to drop post-upstream-clone, and propose_downstream immediately failed with "no spec file", so that doesn't work.

After some 10 other approaches which failed, I now have this hack:

--- packit.yaml
+++ packit.yaml
@@ -1,8 +1,8 @@
 upstream_project_url: https://github.com/martinpitt/cockpit
 actions:
   post-upstream-clone:
-    # packit will overwrite the version in its "fix spec file" stage
-    - tools/create-spec --version 0 --build-all -o cockpit.spec tools/cockpit.spec.in
+    # HACK: packit's "fix spec file" stage does not kick in here, and it does not take the spec from the tarball
+    - sh -exc 'tools/create-spec --version $(git describe | tr - .) --build-all -o cockpit.spec tools/cockpit.spec.in'
   create-archive:
     # The sandcastle doesn't have enough ram to run webpack, so wait
     # until the webpack-jumpstart workflow has run and grab the result.

to build the checkout's cockpit.spec with the expected version number.

With that hack, it worked: proose-downstream log and created Fedora PR

So in summary, I found it really difficult to get propose-upstream working in this scenario (spec file needs to be built). Is there some best practice here?

martinpitt added a commit to martinpitt/cockpit that referenced this issue May 23, 2022
martinpitt added a commit to cockpit-project/cockpit that referenced this issue May 23, 2022
@TomasTomecek TomasTomecek added the area/user-experience Usability issue label May 24, 2022
@TomasTomecek
Copy link
Member

I filed #223 in the past, although your issue is very concrete.

The best practice so far is to manually adjust Version when doing releases. Which won't work for you since you generate the spec file.

If this --version $(git describe) would work for everyone, that would be a win.

@lachmanfrantisek
Copy link
Member

So apparently that "fix spec file" magic does not get applied to propose_downstream stage?

No. The per-job description of actions can be found here: https://packit.dev/docs/actions/#propose-downstream-command

So in summary, I found it really difficult to get propose-upstream working in this scenario (spec file needs to be built). Is there some best practice here?

Anaconda is on the same boat here and we already made a few changes on this front (e.g. packit/packit#1413). (And I must say it's really hard to do correctly to avoid any circular dependencies between steps done during propose-downstream. And be consistent between CLI and service.)

On a more constructive note:

  • Would running a fix-spec-file action during propose-downstream work for you? (I am just worried about breaking the flow for current users so we can maybe use a different name.)
  • We have also been thinking about adding an action to be run in the dist-git repo (before doing a commit).
  • During propose-downstream, we don't touch the spec-file version. We just log a warning if the spec version is smaller than the one got from the git tag. Should we update it? (Ideally in some action that can be redefined.)

Ideally packit would take cockpit.spec from the downloaded release tarball -- everything is correct there. I tried to drop post-upstream-clone, and propose_downstream immediately failed with "no spec file", so that doesn't work.

  • I am not sure if this can be used for all users -- upstream archive does not need to contain a spec-file at all.

@martinpitt
Copy link
Author

Would running a fix-spec-file action during propose-downstream work for you?

I think so -- at least the builtin default does the right thing for COPR, i.e. replace the spec's Version: with the actual version. Packit already knows this, but does not expose it as a variable ($PACKIT_PROJECT_VERSION is not set for post-upstream-clone). As I said, this is not my most liked solution, but it would help to avoid these nasty shell/seddery hacks. FTR, I updated the description with a fixed git command, the previous one didn't work.

I am not sure if this can be used for all users -- upstream archive does not need to contain a spec-file at all.

Right, but if it contains it, then I feel this should be preferred to the version in the cloned git? These days, most upstream projects don't keep the git-tracked spec up to date with the version, that's outright impossible with COPRs from PRs even. Do you think it would be possible to apply the normal "search for a .spec file" approach in the upstream tarball for propose-downstream first, and only then fall back to all the post-upstream-clone:, fix-spec-file etc. magic?

@lachmanfrantisek
Copy link
Member

OK, so we have basically three action items:

  • update the spec-version during a propose-downstream job.
    • What about doing this as a fix-spec-file action? Or a new bump-spec-version so users can disable this?
    • Do we want to use a new version (get from tag) or take the highest from tag and spec-file? Also, we can introduce get-current-version.
    • This might help our tools as well so we don't need to maintain the version in the upstream specfile...
  • Provide PACKIT_PROJECT_VERSION during propose-downstream.
    • We can do that if it makes sense. (With service, it will be a version transformed from git tag using the upstream_tag_template.)
  • Try getting a spec-file from the source archive.
    • But we are getting the info about the archive from the spec-file...;(
    • We can possibly do something with that but I am worried this can mess the already-a-bit-complicated process more.

@lachmanfrantisek
Copy link
Member

Last update of the action items:
(Leaving the spec-from-archive for later):

  • update the spec-version during a propose-downstream job.
    • Do this as a bump-spec-version action so users can disable this.
    • Use the value from get-current-version action or get it from the tag by default.
    • Preserve the warning if upstream-release-monitoring has a new version.
  • provide PACKIT_PROJECT_VERSION env.var. during pre-sync action. (As a value we use the output from the get-current-version action or tag by default. (What we use in spec-file.)
  • make sure the following use-cases work:
    • propose-downstream in service with the spec-file present in upstream (e.g. Packit; spec-file does not need to updated in upstream)
    • propose-downstream in service with the spec-file generated during action (e.g. Anaconda, Cockpit)
    • propose-downstream run via CLI for both upstream-located and action-generated specfile

@csomh
Copy link
Contributor

csomh commented Jun 7, 2022

  • Do this as a bump-spec-version action so users can disable this.

Maybe call this set-spec-version, as versions are not only bumped.

Figure out some way, so that by default this actions doesn't do anything, and it's run only if requested by the user or defined. This way we could maintain backwards compatibility.

@lachmanfrantisek
Copy link
Member

Regarding the syntax, we have multiple options:

  • Nothing by default, the default behaviour when true yaml value is configured, user-defined command otherwise.
    • Here, I am worried about a collision between a dummy command (like true) and some special value (true/1/True/yes/..). Maybe, we can have some explicit special value like packit-default.
  • Have a dedicated config flag so turn this on. (Nothing set means no change, flag set means the action is run with the default to use the version from get-current-version/tag.)

The person who will work on this will see what is better to use.

@lbarcziova
Copy link
Member

Hi, I was looking into the propose_downstream code base and found out that we actually do set the Version in the specfile (here), in case of Packit Service we set here the version that we get from the upstream tag corresponding to the release that triggered the propose_downstream, but we do it after running the post_upstream_clone action, when preparing files to sync.

If I understand correctly, the real problem here is that the archive we try to get is not correct. But checking how you create the specfile in post_upstream_clone, the Source0 is hardcoded after running the action and since we set the Version in specfile correctly later, it is not considered when downloading the sources because the source name is already hardcoded.

Would it be possible for you to use in Source0 the version macro maybe? Or do you still think providing something similar to fix-spec-file for propose_downstream where you can update the Source0 would be the best option?

@martinpitt
Copy link
Author

Would it be possible for you to use in Source0 the version macro maybe?

Absolutely, yes, thanks for spotting! That also explains why we don't run into this problem in e.g. c-podman. Its spec file uses the version macro in the way you mention:

Source0:        https://github.com/cockpit-project/%{name}/releases/download/%{version}/%{name}-%{version}.tar.xz

and does the upstream-side templating through this indirection:

Version:        %{VERSION}

I'll try the same for cockpit.

If that works, I think we can close this -- indeed my initial "Ideally packit would take cockpit.spec from the downloaded release tarball -- everything is correct there" proposal is difficult due to the chicken-and-egg problem.

Thanks @lbarcziova !

@lbarcziova
Copy link
Member

Great! Let me know if there will be any problems after this change and in that case we can still implement some action.

@martinpitt
Copy link
Author

@lbarcziova ! Works! cockpit-project/cockpit#17450 Thanks for your help!

martinpitt added a commit to martinpitt/cockpit that referenced this issue Jun 14, 2022
Source0 can and should be constructed from the `%{version}` macro. This
will allow packit to download the correct source tarball on releases.
This is the approach that we already do in c-podman and friends.

Revert the hack from 7f996df, and drop the now unnecessary `@PATH@`
handling from create-spec.

Fixes packit/packit-service#1515
martinpitt added a commit to martinpitt/cockpit that referenced this issue Jun 15, 2022
Source0 can and should be constructed from the `%{version}` macro. This
will allow packit to download the correct source tarball on releases.
This is the approach that we already do in c-podman and friends.

Revert the hack from 7f996df, and drop the now unnecessary `@PATH@`
handling from create-spec.

Fixes packit/packit-service#1515
martinpitt added a commit to cockpit-project/cockpit that referenced this issue Jun 15, 2022
Source0 can and should be constructed from the `%{version}` macro. This
will allow packit to download the correct source tarball on releases.
This is the approach that we already do in c-podman and friends.

Revert the hack from 7f996df, and drop the now unnecessary `@PATH@`
handling from create-spec.

Fixes packit/packit-service#1515
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/user-experience Usability issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants