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

Files for Flatpak #3121

Merged
merged 26 commits into from
Oct 17, 2024
Merged

Files for Flatpak #3121

merged 26 commits into from
Oct 17, 2024

Conversation

jamescrake-merani
Copy link
Contributor

Description

As mentioned in the fortnightly meeting yesterday, I have added the following files in the data directory which will be used by the Flatpak distribution. These are:

  • The org.sasview.sasview.png file. I had to modify the ball.ico file present in the repository because the format is not supported for application logos (they need to be either SVGs, or PNGs), and the size of the image was too big (they need to be maximum 512x512).
  • the metainfo file, which provides some metadata about SasView.
  • The desktop file which is needed for SasView to appear as an application. This is mostly lifted from the desktop file the current Debian distribution is using.

I have also added Ubuntu 22.04 to the OS release list so that GitHub Actions will build a separate Ubuntu 22.04 distribution. As discussed previously, the Ubuntu 20.04 distribution was not working in the Flatpak. If we still want to maintain the Ubuntu 20.04 distribution, then I'd suggest building for both Ubuntu 20, and 22. Alternatively, we could also drop support for Ubuntu 20.04, and advise users on that operating system to use the Flatpak instead. The Flatpak builder will need a permanent URL from which it can fetch a working distribution of SasView to package.

I have marked this pull request as a draft because I need a description for the metainfo file. I have copied a 'summary' from the website but, looking at other projects, the description is expected to be a bit of a longer overview of the software. @krzywon do you know of any text that would be most appropriate for this? I also realised while writing this that I need to put a version history. I'll probably just put SasView 6 for now as that will be the first version the Flatpak will be packaged for.

How Has This Been Tested?

I have built a Flatpak bundle with these files, and it seems from my testing to integrate into my desktop without any issues. There is a SasView desktop entry which I can search for, and the logo shows up as expected. IT have just set up virtual machines on my laptop so I should be able to test the Flatpak on other Linux operating systems.

Review Checklist:

Documentation (check at least one)

  • There is nothing that needs documenting
  • Documentation changes are in this PR
  • There is an issue open for the documentation (link?)

Installers

  • There is a chance this will affect the installers, if so
    • Windows installer (GH artifact) has been tested (installed and worked)
    • MacOSX installer (GH artifact) has been tested (installed and worked)

Licencing (untick if necessary)

  • The introduced changes comply with SasView license (BSD 3-Clause)

@jamescrake-merani
Copy link
Contributor Author

@jamescrake-merani
Copy link
Contributor Author

I see the builds are failing. Is this because of #3109 ?

@krzywon
Copy link
Contributor

krzywon commented Oct 9, 2024

I see the builds are failing. Is this because of #3109 ?

Yes. This particular issue has been fixed in #3110 which will be merged during the PR hackathon, if not before. This PR should be pointing to, and built off , the release_6.0.0 branch if this is going into the release.

@jamescrake-merani jamescrake-merani changed the base branch from main to release_6.0.0 October 9, 2024 13:53
@jamescrake-merani jamescrake-merani changed the base branch from release_6.0.0 to main October 9, 2024 13:57
@jamescrake-merani jamescrake-merani changed the base branch from main to release_6.0.0 October 9, 2024 14:05
@jamescrake-merani jamescrake-merani changed the base branch from release_6.0.0 to main October 9, 2024 14:26
@jamescrake-merani jamescrake-merani changed the base branch from main to release_6.0.0 October 10, 2024 07:18
@jamescrake-merani
Copy link
Contributor Author

I've changed the branch to the release branch. Had to do a bit of wrangling with Git but I think its all sorted now.

I think for the changelog, its probably best if we redirect users to the website rather than putting this into the metadata because our changelogs are quite long.

I think the only two things outstanding for the metadata are a description, and I also need to include at least one screenshot. According to Flathub's guidance, they need to be taken on Linux so I will probably take my own of SasView after it opens.

Copy link
Contributor

@lucas-wilkins lucas-wilkins left a comment

Choose a reason for hiding this comment

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

LGTM once description is added.

@lucas-wilkins lucas-wilkins marked this pull request as ready for review October 11, 2024 14:32
@krzywon
Copy link
Contributor

krzywon commented Oct 11, 2024

A few TODOs left here:

  • @jamescrake-merani: Move the items in the data directory into the build_tools directory, possibly into their own subdirectory
  • @jamescrake-merani: Include a screenshot of a simple fit to be included with the flatpack
  • @krzywon: Write a longer description of SasView, outlining various features. Done in 7bdc438

Not going to provide a changelog because we have that on the website.
Nothing in particular raises the content rating but we still need this
to avoid validation errors.
@jamescrake-merani
Copy link
Contributor Author

@krzywon Both of the TODO items have now been completed.

Additionally, I have passed the metainfo file into a linter which highlighted a few issues with it. I've managed to fix all of them so it should be good for publishing. You can see the commit history for all of the errors I fixed but there is one issue I would like to highlight. The linter wanted me to include a licence for the metainfo file. I originally selected BSD 3-clause as it is also used for our codebase. However, the linter complained that the licence was not permissive enough. Instead, I have gone for CC0 as I have seen this licence used in other projects. I wanted to ask you if it is ok to include this licence for the metainfo file.

The new directory for all these files is called application_metadata, under the buid_tools directory. This was the name I came up with but if you have a better idea than feel free to push directly to this branch.

I believe everything is in order for this branch to be merged.

@jamescrake-merani
Copy link
Contributor Author

Nvm looks like a test failed. I have left the office but will check this first thing in the morning tomorrow.

@wpotrzebowski
Copy link
Contributor

wpotrzebowski commented Oct 15, 2024

Nvm looks like a test failed. I have left the office but will check this first thing in the morning tomorrow.

Sometimes mac builds get stuck. I've restarted the failed build.

@jamescrake-merani
Copy link
Contributor Author

Ah yes fortunately that seems to be the case. They seem to be all passing now.

@krzywon
Copy link
Contributor

krzywon commented Oct 15, 2024

Additionally, I have passed the metainfo file into a linter which highlighted a few issues with it. I've managed to fix all of them so it should be good for publishing. You can see the commit history for all of the errors I fixed but there is one issue I would like to highlight. The linter wanted me to include a licence for the metainfo file. I originally selected BSD 3-clause as it is also used for our codebase. However, the linter complained that the licence was not permissive enough. Instead, I have gone for CC0 as I have seen this licence used in other projects. I wanted to ask you if it is ok to include this licence for the metainfo file.

I'm asking if this is acceptable and will get back to you soon.

The new directory for all these files is called application_metadata, under the buid_tools directory. This was the name I came up with but if you have a better idea than feel free to push directly to this branch.

LGTM

@krzywon
Copy link
Contributor

krzywon commented Oct 16, 2024

To follow up, the licensing seems fine based on what I've been reading. The only outstanding issue on this PR is the location of the images.

@krzywon
Copy link
Contributor

krzywon commented Oct 16, 2024

@jamescrake-merani - please verify the new screenshot links I used are valid and, if so, merge this.

@jamescrake-merani
Copy link
Contributor Author

@krzywon Unfortunately the new image URL is not valid. When I download it, it appears as corrupted. Upon reading the file, it looks like the HTML of the link gets fetched, and not the image itself. I was having an issue with this originally because GitHub doesn't seem to have a 'raw' button on images like it does for everything else. The link I originally put in there was the only working URL I can find. If there is an issue with it then I'm not sure how else to get the raw image through Github.

@krzywon
Copy link
Contributor

krzywon commented Oct 17, 2024

@krzywon Unfortunately the new image URL is not valid. When I download it, it appears as corrupted. Upon reading the file, it looks like the HTML of the link gets fetched, and not the image itself. I was having an issue with this originally because GitHub doesn't seem to have a 'raw' button on images like it does for everything else. The link I originally put in there was the only working URL I can find. If there is an issue with it then I'm not sure how else to get the raw image through Github.

These files should be resolvable from sasview.org. I'll find the URLs for those and change the links here soon.

@krzywon
Copy link
Contributor

krzywon commented Oct 17, 2024

@jamescrake-merani - can you check again?

@jamescrake-merani
Copy link
Contributor Author

Thanks; this should be fine now, and the linter is happy. I have removed the last one because, as per Flathub's quality guidance, the screenshots should just be of the application running. The welcome screen should be fine since it appears when launching the application but I think the logo won't count as a screenshot.

If the above is fine, then I think this branch is ready to merge. If you're happy then feel free to merge it.

@krzywon krzywon merged commit 8b0da66 into release_6.0.0 Oct 17, 2024
40 checks passed
@krzywon krzywon deleted the files-for-flatpak branch October 17, 2024 13:50
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