Makefile: Add install and install-user targets

[amboar]

Script some of the behaviour previously captured in the readme as instructions to execute manually.

Signed-off-by: Andrew Jeffery andrew@aj.id.au

[mwilck]

Before we merge this, we should clarify if we want to have it on develop only or also on our other branches. In the latter case, we need to create the branches first, and start merging to the "lowest" one we want to support.

[hedayat]

Oops, sorry guys. I wrote my comments few days ago, but didn't know that I should press "Submit review" :D

[DirkHoffmann]

Trying to summarise what was written above: There are three targets,

• install-user : to ~/.local/share/gnome-shell/extensions/contact@projecthamster.org/
• install : system-wide
• dist : create ZIP for submission to extensions.gnome.org

and they should all depend on compile. And install depends on dist.

Sounds plausible to me.

[hedayat]

* `dist` : create ZIP for submission to extensions.gnome.org

and they should all depend on compile. And install* depends on dist.

You already contradicted yourself: install depends on dist, while dist creates an archive for submission to EGO (and install doesn't submit the archive to EGO) :D ;)

Anyway, if you all like that, go with it. Simply, I don't. :)

[benjaoming]

@mwilck

Before we merge this, we should clarify if we want to have it on develop only or also on our other branches. In the latter case, we need to create the branches first, and start merging to the "lowest" one we want to support.

It's true, removing the old instructions from README and replacing them with instructions that only work with the current develop branch isn't ideal.

@amboar What do you think about keeping the old instructions and instead add something like this?

Shortcut on develop

If you are using the develop branch since May 2020, you can run make install-user to install your current working branch in your user environment

[amboar]

To be honest I didn't expect the PR to drive so much discussion. The patch made my life easier testing @mwilck 's 3.36 changes and I figured I might as well propose it. I'm not attached to it and am happy to drop it if we feel it's taking more energy to handle than is necessary.

I feel like applying it to just the develop branch causes a bit of a maintenance headache, though I'm only guessing at how you manage your branches. That said I'm not living with the maintenance problem so I don't mind if you want to pick it up. I'll rework the readme as you suggest @benjaoming.

Addressing the dist vs compile target discussion, reusing dist was the path of least resistance as that was what was described in the readme. I think the question of whether we should conform more accurately to autotools semantics can be sorted out separately? I agree that how dist is implemented here doesn't really align with the dist target of autotools, but is that something we're aiming for?

[amboar]

So with what I've just pushed we now have both install and install-user targets. I've split the patches out so you can take as many as you think are reasonable :) The install-user patch comes before the addition of install because it's more obviously a direct translation of the instructions in the README. The addition of install exposes and uses the DESTDIR variable so we can adjust the target installation path, which I then make use of to (re-)implement install-user. Finally the documentation is updated separate to either patch so if it's still contentious whether the patches should apply to master or not you can pick the others into develop while that discussion continues.

The targets still depend on dist, I haven't tackled the idea of making them instead depend on compile.

[mwilck]

@amboar ,

To be honest I didn't expect the PR to drive so much discussion.

That's how it goes when you make contributions to OSS projects, right? No matter how simple you think it would be, suddenly it gets a big issue :grin:
I'm sorry that I had my part in that, I guess I was a bit overzealous.

Thanks a lot for your contribution.

[amboar]

@amboar ,

To be honest I didn't expect the PR to drive so much discussion.

That's how it goes when you make contributions to OSS projects, right? No matter how simple you think it would be, suddenly it gets a big issue grin I'm sorry that I had my part in that, I guess I was a bit overzealous.

No, don't apologise! I'm just a drive-by contributor and have zero stake in ongoing maintenance, so I would have been fine with any response let alone possibly getting it merged. It just seemed like a simple enough change that I was just expecting "yeah, let's merge" or "nah, reasons, closed" 🙂

Thanks a lot for your contribution.

No worries - thanks for taking the time to look at it and respond.

[benjaoming]

This is a positive improvements that received some suggestions, but no real concerns, so it should be good to go.

(sorry about the extra commit, using Github Suggestions wasn't ideal for quick fingers)

[DirkHoffmann]

Hi guys, This seems to have converged. I just "resolved" (I.e. collapsed) the three "conversations" for cleanup. This PR now needed an assignee, and while every one of the reviewers would be fine, I have randomly choosen the first who answered. Anyone who is faster can change this again. :-) @amboar, just to give you some context, the four reviewers/managers who answered on this ticket are only in charge since March this year. So your proposal came in a period, where we still have to find our way with the github process, and it served as an example for that as well. Thank you very much for your input and fast feedback!