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

create an empty default welcome file #3194

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mcfnord
Copy link
Contributor

@mcfnord mcfnord commented Oct 27, 2023

With this change, a Debian installation will create an empty /etc/jamulus/welcome.html file and specify it as the welcome file in the systemd default configuration.

https://github.com/orgs/jamulussoftware/discussions/3086#discussioncomment-6248420

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

@@ -6,4 +6,7 @@ set -e

adduser --system --quiet --home /nonexistent --no-create-home jamulus

sudo mkdir /etc/jamulus
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will fail if the directory already exists. It will also

Suggested change
sudo mkdir /etc/jamulus
sudo mkdir -p /etc/jamulus

Copy link
Member

Choose a reason for hiding this comment

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

Why is sudo required anyway? If adduser I'd executable meditate should also work without sudo. Nevertheless, I believe there's another way to add files and folders which is then also automatically cleaned up by dpkg/apt. Check out deploy_linux.sh

Copy link
Contributor Author

@mcfnord mcfnord Nov 2, 2023

Choose a reason for hiding this comment

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

Writing in /etc might require root access, but I can't explain why adduser doesn't in this instance. Should I test this without the sudo?

There is no deploy_linux.sh. Do you mean deploy_deb.sh ?

If I am uninstalling an application, there's a case for intentionally not removing a small file the user has customized. I guess a concern is that if I re-install, I don't expect the previous welcome file?

Are there examples of our files that are handled in the auto-cleanup way you prefer?

@@ -6,4 +6,7 @@ set -e

adduser --system --quiet --home /nonexistent --no-create-home jamulus

sudo mkdir /etc/jamulus
sudo touch /etc/jamulus/welcome.html
Copy link
Collaborator

@pljones pljones Oct 27, 2023

Choose a reason for hiding this comment

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

This will require the user to edit the file as root (it is being created by root and both owner and group will be root). The file needs to be readable by the user jamulus, and - depending on how the system is already set up - this may not be the case - it may not be readable others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about:

chmod -c 666 /path/to/file

Everyone can read and write, nobody can execute.

Copy link
Collaborator

@pljones pljones Oct 28, 2023

Choose a reason for hiding this comment

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

I wouldn't want files installed on a system that any user could edit without control, that were then displayed publicly...

Ideally:

  • there would be a jamulus group as well as user
  • the file would be placed in a location owned by the jamulus user and owned by jamulus:jamulus
  • permissions could then be 0660 - so jamulus user and any user in the jamulus group could edit it - but no one else

This is getting outside the scope of this PR, though, so I think just leaving (with -p) will do for now - so only root can edit. It probably is worth making use it's a+r permissions (i.e. anyone can read, other permissions unchanged).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a+r appears to work

Copy link
Member

Choose a reason for hiding this comment

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

Check jamulus.install file and deploy_deb.sh. I think you just need to move the file to the correct location during deb creation. This ensures the file doesn't get overwritten by updates, I suppose

Copy link
Contributor Author

@mcfnord mcfnord Nov 2, 2023

Choose a reason for hiding this comment

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

I don't want my custom welcome file to be overwritten when I upgrade.

Copy link
Member

Choose a reason for hiding this comment

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

Exactly. I think that we might want to add an empty file to the deb directly instead of creating it during install time. But I'm unsure about the actual way config files are handled.

For example I'd like that a apt purge deletes it while an apt remove doesn't.

I found https://wiki.debian.org/ConfigPackages but it doesn't help me.

@mirabilos how should config files be handled correctly in deb files?

Copy link
Contributor

Choose a reason for hiding this comment

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

The usual Debian way: ship it as conffile by placing it in /etc/jamulus/ in the package contents, then it will be placed there on first install by dpkg and later updated with newer versions iff the user did not change it; if the user changed it, they’ll get a chance to merge, keep their changes or throw their changes away. And the file will have proper default root:root 0644 permissions.

Copy link
Contributor Author

@mcfnord mcfnord Nov 3, 2023

Choose a reason for hiding this comment

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

@ann0see, touch won't overwrite anything. The Debian technique would behave more appropriately with apt purge vs. apt remove, and I guess that's good citizenship. Someday deploying default welcome file metadata could be a thing.

Copy link
Member

Choose a reason for hiding this comment

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

@mirabilos thanks for answering.

Co-authored-by: Peter L Jones <pljones@users.noreply.github.com>
@ann0see
Copy link
Member

ann0see commented Nov 1, 2023

Before this gets merged, we should check how newly clients behave. Do they show an empty chat window?

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.

See comments. Also maybe @jujudusud can help out a bit as he knows packaging on Arch (I think).

@mcfnord
Copy link
Contributor Author

mcfnord commented Nov 2, 2023

Before this gets merged, we should check how newly clients behave. Do they show an empty chat window?

Yes. There's no content change in the chat window, visually same as if there's no welcome message. Should I also test that getServerProfile returns same results in result.welcomeMessage in both cases?

@ann0see
Copy link
Member

ann0see commented Nov 2, 2023

Should I also test that getServerProfile returns same results in result.welcomeMessage in both cases?

Yes

@mirabilos
Copy link
Contributor

mirabilos commented Nov 3, 2023 via email

@mcfnord
Copy link
Contributor Author

mcfnord commented Nov 3, 2023

Nobody has ever expressed a great usage of metadata in the welcome file. Instead, our docs now mention bad uses for it. I tried to pitch welcome file metadata that our client reads and uses, but rdica and pljones found this idea meritless. So default content remains a fantasy. But littering a 50 byte file is no joke.

@ann0see
Copy link
Member

ann0see commented Nov 3, 2023

It's still no problem, but we best follow the standard approach.

@mcfnord
Copy link
Contributor Author

mcfnord commented Nov 5, 2023

This will apparently be our first usage of conffiles. How can this be? This command would seem to work the same way:

sudo systemctl edit --full jamulus-headless

It shows an editable configuration file, our ./linux/debian/jamulus-headless.service file. I hope if we change a field, then the user would get the option to merge, discard local changes, etc. But I don't see a file named conffiles.

I do see jamulus.install, and jamulus-headless.install, which maybe have the format of a conffile. jamulus-headless.install has a two-token line separated by whitespace, but the first field does not seem like a fflag (and remove-on-upgrade is the only supported flag anyway). Also, these files maybe don't contain absolute pathnames, because lines don't start with a slash, which is a requirement for conffiles.

Why don't I see a file named DEBIAN/conffile? Is that optional? Are we using this mechanism now?

(Once I was framed for a speech crime. After three nights in jail, I was taken to a room to wait for arraignment, where a judge sets the bail. One by one, each defendant was taken before the judge. As they'd return some said there was a television camera in the courtroom. Why? As we ran out of defendants, a math wiz with an opioid addiction calculated that the cameras must be for me. He said a judge can't show weakness on television. He turned to me, pointed at me, and said, "They must be here for you. You're the sacrificial lamb!"

I hope I'm not the sacrificial lamb for conffiles.)

@ann0see
Copy link
Member

ann0see commented Nov 5, 2023

I don't know the progress of the top of my head, but I remember that the DEBIAN folder is created and populated via the shell script I mentioned. So you'd cp the empty welcome message in there.

  1. create an etc folder
  2. cp the empty file there
  3. that's already in the script, but make sure the deb is built.

@ann0see
Copy link
Member

ann0see commented Nov 5, 2023

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.

@pljones
Copy link
Collaborator

pljones commented May 6, 2024

Closing in favour of #3281

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Waiting externally
Development

Successfully merging this pull request may close these issues.

None yet

4 participants