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

creating extra appdata-icons.xml.gz for SLE15+ (issue #8259) #8379

Closed
wants to merge 4 commits into from
Closed

creating extra appdata-icons.xml.gz for SLE15+ (issue #8259) #8379

wants to merge 4 commits into from

Conversation

doccaz
Copy link
Contributor

@doccaz doccaz commented Sep 18, 2019

Here is my proposed patch to work around the app-icons vs appdata-icons issue with SLE15+. The effect is minimal (only another file published with the same contents as app-icons.xml.gz). These files are usually small, and will allow for the repositories to be used with GNOME Software on both SLE12/Leap 42.3 and Leap 15/SLE15+.

@DavidKang DavidKang added the Backend Things regarding the OBS backend label Sep 18, 2019
@DavidKang
Copy link
Contributor

@doccaz, thanks for your contribution 💐

@DavidKang DavidKang requested a review from mlschroe September 18, 2019 15:17
@doccaz
Copy link
Contributor Author

doccaz commented Sep 18, 2019

Just found out that the extra *-appdata-icons.tar.gz file I created is NOT being deleted by the publisher when it updates the repository. So I now have a couple extra appdata-icons files in the directory... Trying to find where I missed it in the code.

@DavidKang
Copy link
Contributor

@mlschroe or @M0ses, could you have a look to this PR? please 😸

@@ -658,7 +658,11 @@ sub createrepo_rpmmd {
if (-e "$extrep/repodata/app-icons.tar") {
print " adding app-icons.tar to repodata\n";
qsystem($modifyrepo_bin, "$extrep/repodata/app-icons.tar", "$extrep/repodata", @legacyargs) && die(" modifyrepo failed: $?\n");
unlink("$extrep/repodata/app-icons.tar");
qsystem("mv $extrep/repodata/app-icons.tar $extrep/repodata/appdata-icons.tar") && die(" copy appdata-icons failed: $?\n");
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't call qsystem() with "mv xxx yyy" as one string, instead you should use 'mv', 'xxx', 'yyy'.
But in this specific case you shouldn't call qsystem() at all; just use perl's rename() function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Sorry for the (huge) delay. Too much happening at the same time.

@adrianschroeter
Copy link
Member

can you squash the two commits into one? Will merge right afterwards :)

@codecov
Copy link

codecov bot commented Dec 4, 2019

Codecov Report

Merging #8379 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8379      +/-   ##
==========================================
+ Coverage   90.86%   90.89%   +0.02%     
==========================================
  Files         496      496              
  Lines       21022    21022              
==========================================
+ Hits        19101    19107       +6     
+ Misses       1921     1915       -6

@adrianschroeter
Copy link
Member

I have manualy cherry-picked now to avoid the additional merge commit.

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backend Things regarding the OBS backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants