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

Add IO::Uncompress::UnXz to dependencies #4011

Merged
merged 1 commit into from
Aug 4, 2021

Conversation

dcermak
Copy link
Contributor

@dcermak dcermak commented Jul 5, 2021

The UnXz module is required to decompress .xz files which are commonly created
by the kiwi appliance builder on OBS. If this package is not required, then its
availability on the webui is accidental and can cause hard to debug "download"
errors like reported in https://progress.opensuse.org/issues/94997

@perlpunk
Copy link
Contributor

perlpunk commented Jul 5, 2021

Since perl-IO-Compress-Lzma (which provides the module) is not in Leap 15.2 yet, we have to link the Factory package to https://build.opensuse.org/project/show/devel:openQA:Leap:15.2 also.
I can link it if necessary.

@dcermak
Copy link
Contributor Author

dcermak commented Jul 5, 2021

Since perl-IO-Compress-Lzma (which provides the module) is not in Leap 15.2 yet, we have to link the Factory package to https://build.opensuse.org/project/show/devel:openQA:Leap:15.2 also.
I can link it if necessary.

That would be great if you could do that!

@perlpunk
Copy link
Contributor

perlpunk commented Jul 5, 2021

Ok, I added it:

osc linkpac openSUSE:Factory perl-IO-Compress-Lzma  devel:openQA:Leap:15.3

Same for Leap 15.2 and 15.1

dependencies.yaml Outdated Show resolved Hide resolved
@perlpunk
Copy link
Contributor

perlpunk commented Jul 6, 2021

Please see the comments here: https://build.opensuse.org/package/show/devel:openQA:Leap:15.2/perl-IO-Compress-Lzma

So IO::Compress::Base is in perl core, but also available as a CPAN module ("dual life").
IO::Compress::Lzma requires the newer CPAN version.

I don't fully understand why this module is problematic, as there are other dual life modules in Factory.

But it looks like this can't be resolved quickly, if at all. So we would have to make this module optional for now.
If we really need xz for Leap, we might have to resort to the command line tool.

@perlpunk
Copy link
Contributor

perlpunk commented Jul 6, 2021

We're trying to link packages from devel:languages:perl. Stay tuned...

@perlpunk
Copy link
Contributor

perlpunk commented Jul 6, 2021

Ok, packages linked. Look into the ticket for the details. OBS seems to be happy now

@AdamWill
Copy link
Contributor

AdamWill commented Jul 6, 2021

Hi! waves Other people using openQA here, not just SUSE.

Requiring something just because OBS produces images in that format seems like a poor choice. It's not a dependency of openQA, it's a thing used in a specific workflow that uses openQA. This should be a recommendation at best. If there's no implementation of soft dependencies, it should be nothing.

edit: seems to me a much more appropriate fix for "confusing behaviour when compression/decompression library for a specific asset unavailable" would be to make that behaviour better, not just hard require every compression/decompression library you arbitrarily decide is important.

@Martchus
Copy link
Contributor

Martchus commented Jul 6, 2021

Maybe it would be better to make it an optional dependency, indeed. We can also add it as dependency of a meta package like the one we already have for the openSUSE test distribution.

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

Hi! waves Other people using openQA here, not just SUSE.

Requiring something just because OBS produces images in that format seems like a poor choice

@AdamWill That's a good point. Normally we just use the openSUSE package as an example of one Linux distribution showing us what's needed upstream here. In this PR seems we have gone to far requiring what openQA does not really require

@dcermak
Copy link
Contributor Author

dcermak commented Jul 7, 2021

So how should we go forward with this? Just close it?

@kraih
Copy link
Member

kraih commented Jul 7, 2021

Weird how Red Hat specific patches like #4006 get approved, and SUSE specific ones like this one not.

@Martchus
Copy link
Contributor

Martchus commented Jul 7, 2021

To be fair, #4006 is about compatibility with Mojo < 9 in general and this PR is about supporting XZ in general. Both requirements are not necessarily distribution-specific.

@kraih
Copy link
Member

kraih commented Jul 7, 2021

The arguments deciding both are about Red Hat packaging though. That's the stated reason why this PR was blocked. It feels very inconsistent.

@okurz
Copy link
Member

okurz commented Jul 7, 2021

how about just one line of "Recommends" in the spec file, like for perl(Mojolicious::Plugin::OAuth2)?

@AdamWill
Copy link
Contributor

AdamWill commented Jul 7, 2021

The arguments deciding both are about Red Hat packaging though. That's the stated reason why this PR was blocked. It feels very inconsistent.

Um. My argument didn't refer to Red Hat at any point. It's perfectly simple: this is not a dependency of openQA. openQA does not need it to function correctly or optimally. It should not be listed as a dependency.

@AdamWill
Copy link
Contributor

AdamWill commented Jul 7, 2021

how about just one line of "Recommends" in the spec file, like for perl(Mojolicious::Plugin::OAuth2)?

That would be better, though it still seems like a wrong way to "fix" the problem. Why not just improve openQA's behaviour in general when it's being asked to decompress or compress something, but the necessary module isn't available? That would improve things for any similar case, rather than just working around this specific case.

@okurz
Copy link
Member

okurz commented Jul 8, 2021

Why not just improve openQA's behaviour in general when it's being asked to decompress or compress something, but the necessary module isn't available?

Improve how? Like automatically install from CPAN?

@AdamWill
Copy link
Contributor

AdamWill commented Jul 8, 2021

Like log the problem and the appropriate resolution correctly. The initial description here says:

"If this package is not required, then its availability on the webui is accidental and can cause hard to debug "download" errors like reported in https://progress.opensuse.org/issues/94997"

There seem to be two angles there: I'm not quite sure what "its availability on the webui is accidental" means, but if it's "accidental", we can fix that, no? And if it causes "hard to debug "download" errors", we can make those errors easier to debug, no?

@okurz
Copy link
Member

okurz commented Jul 8, 2021

And if it causes "hard to debug "download" errors", we can make those errors easier to debug, no?

Right, #4010 might solve that already.

@dcermak
Copy link
Contributor Author

dcermak commented Aug 3, 2021

So, how should we move forward with this? I don't want to force the addition of another pointless dependency. On the other hand being able to decompress ISOs and HDDs is one of the core functionalities that openQA provides and thus I see a certain legitimacy in it depending on a decompression library.

@okurz
Copy link
Member

okurz commented Aug 3, 2021

@dcermak nobody questioned that openQA should be able to decompress what it promises. This is just how to handle dependencies. If it's just for o3 of course we can install it there. For the package I recommend a recommend, see #4011 (comment)

@AdamWill
Copy link
Contributor

AdamWill commented Aug 3, 2021

FWIW, when there's something we need installed for Fedora's openQA instances but it's not really a dependency of openQA per se, I put it in the ansible deployment plays we use. How are the SUSE official instances deployed? Could you put it there?

Aside from that, yeah, a Recommends: or Suggests: would seem less wrong.

@okurz
Copy link
Member

okurz commented Aug 3, 2021

@AdamWill yes, that is what I mean as alternative just providing it for the administrated instances. We use salt for the SUSE internal instance openqa.suse.de

@dcermak
Copy link
Contributor Author

dcermak commented Aug 4, 2021 via email

@dcermak
Copy link
Contributor Author

dcermak commented Aug 4, 2021

The CI is now failing as it cannot install perl-Mojo-SQLite-3.006… Should this be fixed by #4105?

@okurz
Copy link
Member

okurz commented Aug 4, 2021

yes

@okurz
Copy link
Member

okurz commented Aug 4, 2021

please rebase after #4105 was merged

The UnXz module is required to decompress .xz files which are commonly created
by the kiwi appliance builder on OBS. If this package is not required, then its
availability on the webui is accidental and can cause hard to debug "download"
errors like reported in https://progress.opensuse.org/issues/94997
@codecov
Copy link

codecov bot commented Aug 4, 2021

Codecov Report

Merging #4011 (47dfa55) into master (c623b63) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4011      +/-   ##
==========================================
- Coverage   97.79%   97.78%   -0.02%     
==========================================
  Files         371      371              
  Lines       33118    33118              
==========================================
- Hits        32389    32385       -4     
- Misses        729      733       +4     
Impacted Files Coverage Δ
t/05-scheduler-full.t 98.40% <0.00%> (-1.60%) ⬇️
lib/OpenQA/Worker/WebUIConnection.pm 93.17% <0.00%> (-0.98%) ⬇️
t/lib/OpenQA/Test/Utils.pm 82.05% <0.00%> (-0.29%) ⬇️
lib/OpenQA/Worker/Job.pm 99.70% <0.00%> (+0.14%) ⬆️
lib/OpenQA/Scheduler.pm 100.00% <0.00%> (+1.85%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c623b63...47dfa55. Read the comment docs.

@mergify mergify bot merged commit 3bfa5b3 into os-autoinst:master Aug 4, 2021
@AdamWill
Copy link
Contributor

AdamWill commented Aug 4, 2021

no objections here, thanks for considering the feedback.

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.

6 participants