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

pkglistgen: cli: Explicitly enable product-composer instead of detecting directories. #3091

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

gleidi-suse
Copy link
Contributor

This PR is based on #3089

Copy link

codecov bot commented May 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 54 lines in your changes missing coverage. Please review.

Project coverage is 27.98%. Comparing base (408936d) to head (6d3b8a8).
Report is 7 commits behind head on master.

Files Patch % Lines
pkglistgen/tool.py 0.00% 43 Missing ⚠️
pkglistgen/cli.py 0.00% 6 Missing ⚠️
pkglistgen/engine.py 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3091      +/-   ##
==========================================
- Coverage   28.01%   27.98%   -0.03%     
==========================================
  Files          86       87       +1     
  Lines       14987    15003      +16     
==========================================
  Hits         4199     4199              
- Misses      10788    10804      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Vogtinator
Copy link
Member

Why?

@gleidi-suse
Copy link
Contributor Author

Why?

pkglistgen as it is on master is broken on product-composer based products (SL Micro 6.0 and 6.1 currently). Running

./pkglistgen.py --dry   --verbose -A https://api.suse.de/ update_and_solve --project SUSE:SLFO:Products:SL-Micro:6.1 --scope target --git-url https://src.suse.de/products/SL-Micro#6.1 --force

yields

Cloning into '/home/giacomo/.cache/openSUSE-release-tools/pkglistgen/api.suse.de/SUSE:SLFO:Products:SL-Micro:6.1'...
remote: Enumerating objects: 120, done.
remote: Counting objects: 100% (120/120), done.
remote: Compressing objects: 100% (118/118), done.
remote: Total 120 (delta 41), reused 0 (delta 0), pack-reused 0
Receiving objects: 100% (120/120), 300.19 KiB | 2.13 MiB/s, done.
Resolving deltas: 100% (41/41), done.
Traceback (most recent call last):
  File "/home/giacomo/code/program-management/openSUSE-release-tools/./pkglistgen.py", line 8, in <module>
    sys.exit(app.main())
             ^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/cmdln.py", line 261, in main
    return self.cmd(args)
           ^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/cmdln.py", line 284, in cmd
    retval = self.onecmd(argv)
             ^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/cmdln.py", line 422, in onecmd
    return self._dispatch_cmd(handler, argv)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/site-packages/cmdln.py", line 1123, in _dispatch_cmd
    return handler(argv[0], opts, *args)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/giacomo/code/program-management/openSUSE-release-tools/pkglistgen/cli.py", line 117, in do_update_and_solve
    return solve_project(target_project, scope)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/giacomo/code/program-management/openSUSE-release-tools/pkglistgen/cli.py", line 99, in solve_project
    return self.tool.update_and_solve_target(api, target_project, target_config, main_repo,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/giacomo/code/program-management/openSUSE-release-tools/pkglistgen/tool.py", line 766, in update_and_solve_target
    file_utils.unlink_all_except(product_dir)
  File "/home/giacomo/code/program-management/openSUSE-release-tools/pkglistgen/file_utils.py", line 32, in unlink_all_except
    name_path = os.path.join(path, name)
                ^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen posixpath>", line 76, in join
TypeError: expected str, bytes or os.PathLike object, not NoneType

This is because update_and_solve_target tries to handle every kind of product (both legacy product builder / converter and current product composer) with a single code path. There should probably be different code paths implementing the same interface to handle different kind of products.

This PR is not final and just an attempt at having correctly generated pkglists on SL Micro 6.1 . If it works we can work on a more structured proposal. What do you think?

@Vogtinator
Copy link
Member

This PR is not final and just an attempt at having correctly generated pkglists on SL Micro 6.1 . If it works we can work on a more structured proposal. What do you think?

Sure. For now just this particular failure should be guarded IMO

@gleidi-suse gleidi-suse force-pushed the explicit_product_composer branch 4 times, most recently from e7daea5 to adfba88 Compare May 28, 2024 13:48
@g7
Copy link
Member

g7 commented May 29, 2024

I've tested this in one of the SLE 15 SP6 stagings, old behaviour ("legacy engine") works as expected.

@gleidi-suse gleidi-suse marked this pull request as ready for review May 29, 2024 12:21
@gleidi-suse
Copy link
Contributor Author

I know I said I'd work on a more structured proposal but we need this running on SLFO stagings ASAP.

In a perfect universe we should really get rid of all if git_url and if use_product_compose by having different objects encapsulating git/svn and product-builder/product-composer operations exposing the same interface . In any case it is not required to deliver so I'll do that in the future, as they can be implemented in a backward compatible way now that we have --engine .

What do you think?

@Vogtinator
Copy link
Member

If you need a quick fix for quick deployment, I'd prefer a as small as possible fix to address only the immediate exception.

Bigger changes like this need a more in-depth review...

@gleidi-suse gleidi-suse force-pushed the explicit_product_composer branch 2 times, most recently from 245e1c6 to 33c0b44 Compare May 30, 2024 07:49
Copy link
Member

@g7 g7 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, with some nitpicks.

I want to highlight though the write_all_groups() method not being called anymore in product-compose mode. Which is fine to me, but it's currently different with what is in master right now.

Either way, tested against a SLE15SP6 staging (in "legacy" mode) and the output has been what I expected. Thanks!

pkglistgen/cli.py Outdated Show resolved Hide resolved
pkglistgen/cli.py Outdated Show resolved Hide resolved
pkglistgen/tool.py Outdated Show resolved Hide resolved
pkglistgen/tool.py Outdated Show resolved Hide resolved
pkglistgen/tool.py Outdated Show resolved Hide resolved
@gleidi-suse gleidi-suse force-pushed the explicit_product_composer branch 3 times, most recently from bc47adc to 3680551 Compare June 3, 2024 13:29
Copy link
Member

@g7 g7 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments! 💯

Aside from the one-line change in my latest comment (self.product_dir -> product_dir), it works as expected against a SLE15SP6 staging.

I will schedule a full target build so we can check that as well.

pkglistgen/tool.py Outdated Show resolved Hide resolved
@gleidi-suse
Copy link
Contributor Author

@Vogtinator Could you please give an in depth review to this PR? It was tested on SL Micro 6.1, SLFO Stagings and SLCS 6.0 and it is currently running for SLE 15 SP6 's target. Thank you

@Vogtinator
Copy link
Member

@Vogtinator Could you please give an in depth review to this PR? It was tested on SL Micro 6.1, SLFO Stagings and SLCS 6.0 and it is currently running for SLE 15 SP6 's target. Thank you

Can do, but earliest tomorrow

@g7
Copy link
Member

g7 commented Jun 5, 2024

Tested on SLE15SP6 target (sorry, it took a rather long time), and the output is as expected.

@gleidi-suse
Copy link
Contributor Author

@Vogtinator were you able to take a look at this PR? Thank you

@gleidi-suse gleidi-suse force-pushed the explicit_product_composer branch 2 times, most recently from 030339d to b883f63 Compare June 10, 2024 07:58
Copy link
Member

@g7 g7 left a comment

Choose a reason for hiding this comment

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

SP6 Staging and Targets work fine, my remarks were addressed, flake8 is happy: looks good to me

@dirkmueller dirkmueller merged commit dc9d44d into openSUSE:master Jun 11, 2024
11 of 13 checks passed
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.

4 participants