search

RPi-Distro / raspberrypi-sys-mods

A collection of Raspberry Pi-sourced system configuration files and associated scripts
99 stars 36 forks source link

resolves RPi-Distro/raspberrypi-sys-mods#42: prompts user for third pā€¦ #51

Closed BitBistro closed 3 years ago

BitBistro commented 3 years ago

resolves RPi-Distro/raspberrypi-sys-mods#42

Please consider this patch that would prompt users if they would like to install third party repositories. If the vscode repo already exists no attempt will be made to change it regardless to the answer of the question. User should be prompted once, or if they rerun dpkg-reconfigure.

The hooks added will allow future debconf questions to be added relatively easily. It should also be setup for localization, but I can only confidently add English text.

Also, slightly modified the way we write out the key. It is first written to a temp file instead of what ever the current directory of the process is. It is then moved as normal and chmod'd.

BitBistro commented 3 years ago

I should note, the default is true: add the repo. The user can easily preload debconf with the response. There are tutorials online on how to do this, but I could write something up if needed.

MichaIng commented 3 years ago

Sorry for doing a code comment, this was actually not the right place since it is a general concern about the aim of this PR:

Since the VS Code repo has been added now for days already, and the postinst code won't add it a second time, it is probably not effective for that matter anymore and will only force an unnecessary debconf for a decision that may have been made already (I removed the repo and key, decided to do so, with this PR I'm asked again about this matter), but it serves as a template and reminder about how such may be done for 3rd party repositories in the future in the first place.

BitBistro commented 3 years ago

Thank you everyone for the comments. I moved the default to no, and set the priority to high. Regardless of what I set these to the maintainers can change as they see fit; so I suppose I have defeated my own argument. From my standpoint it seems like this is what the community would prefer.

@MichaIng - No problem about the comments. I guess you could comment directly at my fork or just email me. I too feel like this type of functionality might be better for some other package. Personally, I was thinking raspi-config or integrated into the recommend software desktop application. That said, my goal was just provide a better method given the current implementation. I'm a true believer in iterative development. It was put in this package for some reason. It could still be moved.

Also, @MichaIng, I don't think it is too late. Sure a lot of people have gone through and already added this source. A lot have not. Hopefully, there are also many more future installs of Raspberry PI. So even though this isn't a retroactive fix, it is in place for posterity.

I guess I never stated why I did this. It actually isn't about Microsoft, but about user choice. I don't think a package should pull in any third party repository without user consent.

Why isn't it retroactive or bigger? This is just a hobby for me. I have a day job and a family. As it is, I had to learn how to use debconf and build this package. It was done with love for the community and the Pi.

@XECDesign - Can you please comment on this PR. I am interested in your thoughts as well, if you would like me to invest more time in it some way. Or if you feel I should hang up my hat on this and call it day. If you want to keep it off thread, please email me; I'll keep it confidence unless you say otherwise.

Mike Perry mike@bitbistro.org

MichaIng commented 3 years ago

Hopefully, there are also many more future installs of Raspberry PI. So even though this isn't a retroactive fix, it is in place for posterity.

Just to clarify that point. The current upstream code does not install the repository on a fresh package install. Only package upgrades from existing pre-20210125 installs will have the repo installed. This even means that with current upstream pi-gen code, creating a fresh Raspberry Pi OS image would result in this repo never being added. So this PR makes pi-gen users and such, who went through the upgrade already, being faced with the repo again, while currently they wouldn't . This is no killer argument, since a transparent way of adding such a repo, including a way to chose against it once and forever, interactively and non-interactively (path-exclude, debconf-set-selection, ...) is much less of an issue. But it is a question whether one wants to (re)awaken the topic for users where it is no topic (anymore), especially after the massive negative and heated feedback this caused, where probably even a good way of dealing with it might wake negative emotions šŸ˜….

At the day the upgrade arrived I was about to write a commit to make it a regular config file, which is added transparently and can easily be blocked and custom changes/removal are persistent unless one uses DPkg force-confmiss/new options. After recognising the above fact, I decided against finishing the commit, as long as the postinst code keeps behaving like it does now.

However, the more I think about it, the more I agree that debconf indeed would have been the best way, so even if it makes e.g. my systems facing another prompt, unless I pro-actively apply debconf-set-selection, it ads a great template that can be used for future sensitive changes as well. In theory it should be even possible to automatically answer the question with "no", if the package version is >=20210125 and the repo does not exist?

Btw:

If the vscode repo already exists no attempt will be made to change it regardless to the answer of the question.

Actually, given that there are likely users who didn't even recognise it, IMO it would make sense to ask, (especially) if the repo exists already and no answer has been given yet, and to remove it if the answer is "no". If the repo does not exist, but the package version is >=20210125 already, it must have been removed manually, hence the question is obsolete and "no" can be assumed. That way, all my above concerns would be dealt šŸ™‚.

However, I then still think instead of:

yes: new third party repositories may be added

it should be :

yes: the Microsoft VS Code third party repository may be added

But let's see what maintainer(s) thing of the general idea to use debconf.

BitBistro commented 3 years ago

@MichaIng - I didn't have an opportunity to do a fresh install and see the behavior. If the fresh install starts with a package prior to the 20210125 version, then I believe the repo would be added. I'd have to run some tests to see exactly how dpkg --compare-versions "${2}" lt-nl "20210125" would evaluate. I think this will evaluate true for older versions or empty version.

Have you run a fresh install to see the behavior w/out my patch? I have a new Pi I'm building so I might be able to do some testing soon. Time is a resource. If you can say for certain that a fresh install after an update does not have the repo then I can change my branch to include the dpkg compare check. Then behaviors should match. Although I did attempt to change my default here, and there is of course the preload option. But yes, I agree making not a topic to argue about would be good.

I tried to make it not specific to the Microsoft Repo and call it third party. If I am going to call out the specific repo, then it should be a multi-select like ca-certs uses. I was trying to keep it simple. I can improve up on this if we decide to move forward. I'm relucant to put any more effort into it until I hear further. If that is the case too, lets make sure that this package is the right spot.

MichaIng commented 3 years ago

"${2}" lt "20210125" would evaluate true for empty versions. "${2}" lt-nl "20210125" evaluates false for empty versions.

Check out the man page (search for "--compare"): https://manpages.debian.org/dpkg And you don't require a new system to test it, purge the package and install it again šŸ˜‰.

If the fresh install starts with a package prior to the 20210125 version, then I believe the repo would be added.

Ah that is true, so it's installed with the current Raspberry Pi OS images from January 11. I'm still waiting to the pi-gen patch to have it added for new RPi OS builds as well.

I tried to make it not specific to the Microsoft Repo and call it third party. If I am going to call out the specific repo, then it should be a multi-select like ca-certs uses. I was trying to keep it simple.

If further repos are about to be added, I agree a multi-select would be better than multiple yes/no prompts, as long as there is enough space for some info about each repo. It's just that I would never hit "yes" on a question whether generally the package is allowed to add 3rd party repos, unattended from now on, but I want to be able to decide it for each repo individually. If I like to use VS Code, okay, but that doesn't mean that I need or want the Docker repository added automatically in the future, let's say. Currently the questions sounds like it would permit exactly that.

BitBistro commented 3 years ago

@MichaIng Well that was an interesting endevor. I forgot that updating my branch will automatically update the PR. Good lesson., but I do love to make commits. Anyway, I tested the behavior and with or without the dpkg compare check the user is prompted for new installs without the package existing. However, I believe most pi distros will start with some base version, which will either add or not add the repos depending on their version/date.

However I ran into some poor behavior where dpkg-reconfigure will not add the repo if the user the user changes their answer from no to yes. Then I remembered that was why I removed the compare in the first place.

So in order to cast the widest net, and I must say it is impossible to make everyone happy, I added a purge option. So the options are now as follows (with default no):

When purging I do a very rudamentary attempt at seeing if they changed the files. If they did, I do not purge. I use cksum, which is horrible for security implications, but good enough to do this job. It should be installed even on very barebones systems as it is in coreutils which has a required priority. It also should not tax the cpu much. These files are small anyway.

So I said that was it unless I hear more from the maintainers so this is probably it.

XECDesign commented 3 years ago

Since our aim is to remove hurdles and make things simple for beginners who may not know what a repo is, requiring input like that is just confusing.

I can see that you've put time in this commit and it's a great example of how to use debconf, but it doesn't fit with what we're going for, I'm sorry.

robertoschwald commented 3 years ago

I can see that you've put time in this commit and it's a great example of how to use debconf, but it doesn't fit with what we're going for, I'm sorry.

Interesting. What are you going for? Own m$ package, leave _asis, ... ?

XECDesign commented 3 years ago

We'll see.

MichaIng commented 3 years ago

make things simple for beginners who may not know what a repo is

The question can be rephrased as it is simply about whether they what to be able to more easily install Microsoft VS Code or not. If someone does not know what this is, or simply does not want to use it, one does not need to understand what a repo is, but can fully qualified answer "Thanks, not needed", that's all.

XECDesign commented 3 years ago

Sure and maybe that will be something we might do, but there are too many questions up in the air right now.

BitBistro commented 3 years ago

@XECDesign I know your didn't ask for this so I wasn't sure what to expect. Coding is in my blood and I've been cooped up at home because of covid I've needed outlets.

After going down this path my recommendation is to at least add a way to raspi-config to remove the repo. That may be a better place for the add to live to.

There is also the gui package that does the recommended software packages. That depends on things like package-kit that should be able to help handle adding and removing the repo. That could be even better because I don't think lite versions of the Pi even have that package.

Finally, please reach out to me if you want some help with something @XECDesign. I'm not sure what the protocol is for nmu pull requests. I ran into a few other oddities though in other packages and can probably put in some time for other fixes.

XECDesign commented 3 years ago

Thank you very much for your kind approach.

When it comes to this particular issue, at this point, all options are on the table, including removing the repo entirely. That's the only reason I can't give any helpful comments or accept any pull requests right now - it may all become moot.

Contributions are certainly welcome, but they require review so I find that sometimes issues and PRs hang around much longer than they would in the ideal world. I wouldn't want somebody to send a pull yet another pull request and then have it hang for years because of higher priority tasks.

Maybe open issues for the oddities you've spotted over at https://github.com/RPi-Distro/repo/ and then we can go from there?

In general if it's something like a lintian warning or something that has no practical downsides, it's near the bottom of the list. If it's something that affects niche use cases or the PR is particularly complicated and has potential to mess things up elsewhere, it may take a while to get to as well.