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

Test for Ubuntu and Ubuntu version #3199

Merged
merged 31 commits into from
Dec 8, 2023
Merged

Conversation

houbsta
Copy link
Contributor

@houbsta houbsta commented Nov 13, 2023

Short description of changes

CHANGELOG: Build: Ensure apt version >=2.4 to prove that the system is Debian based (using apt and dpkg) and can validate the repo

Context: Fixes an issue?

Fixes: #3198

Does this change need documentation? What needs to be documented and how?

No

Status of this Pull Request

Basic proof of concept for setup-repo.sh

What is missing until this pull request can be merged?

Has been fully tested on Ubuntu 20.04, 22.04 and MacOS 13.6.

Would require testing on a couple of Debian boxes.

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@houbsta
Copy link
Contributor Author

houbsta commented Nov 16, 2023

I have code to also test for the presence of apt, and to get the version number. Which version is the minimum you consider to be required for the repository to work?

On Ubuntu 20.04 the typical apt version is 2.0.9
On Ubuntu 22.04 the apt version is 2.4.11

I could test for apt >= 2.4.x since there are no 2.1.x - 2.3.x branches.

linux/setup_repo.sh Outdated Show resolved Hide resolved
Removed checks for lsb_release (to get distro name) and Ubuntu version. Assuming apt from at least the 2.4 branch will suffice.
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Some more comments on more extensive error messages.

CC: @gilgongo

linux/setup_repo.sh Outdated Show resolved Hide resolved
linux/setup_repo.sh Outdated Show resolved Hide resolved
houbsta and others added 3 commits November 18, 2023 12:16
Co-authored-by: ann0see <20726856+ann0see@users.noreply.github.com>
Co-authored-by: ann0see <20726856+ann0see@users.noreply.github.com>
@ann0see
Copy link
Member

ann0see commented Nov 18, 2023

I could test for apt >= 2.4.x since there are no 2.1.x - 2.3.x branches.

This might hold true for Ubuntu but might not for some other distribution.
I think the broken versions were mentioned in the issue.

https://bugs.launchpad.net/ubuntu/+source/apt/+bug/1950095

Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Thanks. Tested on Debian Stable and it seems good.

linux/setup_repo.sh Outdated Show resolved Hide resolved
linux/setup_repo.sh Outdated Show resolved Hide resolved
linux/setup_repo.sh Outdated Show resolved Hide resolved
linux/setup_repo.sh Outdated Show resolved Hide resolved
@ann0see ann0see added this to the Release 3.11.0 milestone Nov 19, 2023
@ann0see
Copy link
Member

ann0see commented Nov 19, 2023

Also check the code style check.

@ann0see ann0see added needs documentation PRs requiring documentation changes or additions and removed needs documentation PRs requiring documentation changes or additions labels Nov 19, 2023
Don't test for exit code but use an if construct with test instead
Add question to proceed anyway and quote issue URI.
The commit for fixing the issue was included in apt 2.2 (indeed apt >=2.1.15  contains the commit but testing for at least the 2.2.x branch seems sensible)
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Sorry for being picky.

linux/setup_repo.sh Outdated Show resolved Hide resolved
linux/setup_repo.sh Outdated Show resolved Hide resolved
Co-authored-by: ann0see <20726856+ann0see@users.noreply.github.com>
@houbsta
Copy link
Contributor Author

houbsta commented Nov 20, 2023

No need to apologise. It's nice to get it as clear as possible

@houbsta
Copy link
Contributor Author

houbsta commented Nov 23, 2023

I'm embarrassed about how many commits were necessary for a relatively simple change. If the new format (which I believe I already had, but there you go, maybe one indent wrong, who knows) doesn't validate then I'll just abandon this. It's not worth everyone's time for such a simple change on a script that worked and was tested many commits ago... to fail for minor indentation.
This is a bash script, not main code for the project.

@ann0see
Copy link
Member

ann0see commented Nov 23, 2023

Thanks for your comment. I'll take care of the styling.
I think the styling CI must be improved then...

@ann0see
Copy link
Member

ann0see commented Nov 23, 2023

SRY. But it seems to be ok now. I think you can run the beautifier on your device.
Anyways.
It's ok now, I think.

Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

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

Seems good like that. Needs further testing by somebody else too before merging.

@houbsta
Copy link
Contributor Author

houbsta commented Nov 23, 2023

I downloaded and ran shfmt on the code and locally it tested OK. I even ran the code through it and pasted that, but I don't have a reference to the coding style. Tried tab (shfmt file.sh > newfile.sh and shfmt -i 4 ...), tried 4 spaces indents (as per instructions on coding found on here).

However I'm pasting into the web interface, maybe that's what caused it. In any case thanks for fixing the styling; it seems I should have got a whole git setup going from the command line but I think I have to update my auth credentials and could do this easily just using the web editor. I was wrong!

@ann0see
Copy link
Member

ann0see commented Nov 23, 2023

I'm also not sure about the reference for shfmt unfortunately. I thought it uses the default, but maybe not. Probably worth more investigation.

@ann0see
Copy link
Member

ann0see commented Nov 25, 2023

Todo:
Review and testing from second maindev.

@ann0see ann0see linked an issue Nov 25, 2023 that may be closed by this pull request
@ann0see
Copy link
Member

ann0see commented Nov 28, 2023

@pljones @gilgongo Could someone please have a look at this?
Before it's merged we should squash it.

linux/setup_repo.sh Outdated Show resolved Hide resolved
as it should be

Co-authored-by: Peter L Jones <pljones@users.noreply.github.com>
linux/setup_repo.sh Outdated Show resolved Hide resolved
@gilgongo
Copy link
Member

@pljones @gilgongo Could someone please have a look at this? Before it's merged we should squash it.

Works for me on Ubuntu 22.04 apt 2.4.11. Downgraded to 2.4.5 and still works. How do I test it with 2.0.x branch?

@houbsta
Copy link
Contributor Author

houbsta commented Nov 30, 2023

Ubuntu 20.04 should fail

@gilgongo
Copy link
Member

gilgongo commented Dec 1, 2023

Ubuntu 20.04 should fail

OK that fails:

Your apt version is incompatible. You may not be able install this repository.
See: https://github.com/orgs/jamulussoftware/discussions/3180
Also of interest: https://bugs.launchpad.net/ubuntu/+source/apt/+bug/1950095
Please update your OS or use the .deb package from the Website to manually install Jamulus.
Do you wish to attempt to install the repository anyway?
(not recommended, as you might need to fix your apt configuration)
1) Yes
2) No

One thing that occurs to me is that "Your apt version is incompatible." might imply that Jamulus itself won't run. Should the word order be switched to make it clear that it's just the repo we're talking about? So perhaps:

This repository is not compatible with your apt version.
You can install Jamulus manually using the .deb package from
https://github.com/jamulussoftware/jamulus/releases or update your OS.
For more information see: https://github.com/orgs/jamulussoftware/discussions/3180
Also of interest: https://bugs.launchpad.net/ubuntu/+source/apt/+bug/1950095

Sorry, I wasn't sure what the exact context of the wording was until now.

As per suggestion
@houbsta
Copy link
Contributor Author

houbsta commented Dec 1, 2023

Ubuntu 20.04 should fail

OK that fails:

Your apt version is incompatible. You may not be able install this repository.
See: https://github.com/orgs/jamulussoftware/discussions/3180
Also of interest: https://bugs.launchpad.net/ubuntu/+source/apt/+bug/1950095
Please update your OS or use the .deb package from the Website to manually install Jamulus.
Do you wish to attempt to install the repository anyway?
(not recommended, as you might need to fix your apt configuration)
1) Yes
2) No

One thing that occurs to me is that "Your apt version is incompatible." might imply that Jamulus itself won't run. Should the word order be switched to make it clear that it's just the repo we're talking about? So perhaps:

This repository is not compatible with your apt version.
You can install Jamulus manually using the .deb package from
https://github.com/jamulussoftware/jamulus/releases or update your OS.
For more information see: https://github.com/orgs/jamulussoftware/discussions/3180
Also of interest: https://bugs.launchpad.net/ubuntu/+source/apt/+bug/1950095

Sorry, I wasn't sure what the exact context of the wording was until now.

I have made the updates as per your suggestion, good idea.

@houbsta
Copy link
Contributor Author

houbsta commented Dec 2, 2023

I literally just copy/pasted in the web editor without changing anything, then saved, keeping all spacing identical (in theory). Clearly the web editor breaks coding style no matter what I do. What a nightmare.

@ann0see
Copy link
Member

ann0see commented Dec 5, 2023

Do you see the CI output? It tells me that the if has wrong spacing. So click the Checks tab and the code style check. That's how I found the problems

@ann0see ann0see merged commit 135e944 into jamulussoftware:main Dec 8, 2023
10 of 13 checks passed
@ann0see
Copy link
Member

ann0see commented Dec 8, 2023

All good. Merged and thanks for the patience

@pljones pljones added the tooling Changes to the automated build system label Dec 9, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling Changes to the automated build system
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Detect Ubuntu version in setup_repo.sh
4 participants