ronivay / XenOrchestraInstallerUpdater

Xen Orchestra install/update script
GNU General Public License v3.0
1.16k stars 189 forks source link

Add ability to customize xo-src with local patches #146

Closed ixs closed 1 year ago

ixs commented 1 year ago

Some users prefer to use some locally maintained patches to customize xo. Allow for these patches to be stored in the ./patches subdir and applied to the xo git checkout during build.

This is an... "advanced" feature... Not sure how you feel about it considering the last sentence of your readme. I think not doing any patching of the sources is the right approach, but offering the ability for interested users to apply their own patches is sensible. Support effort should be nil, as people able to build their own patches can also fix them.

olivierlambert commented 1 year ago

I suppose you are aware that Xen Orchestra is aGPLv3? (not just GPL). Basically, this means any modification to it should be also published, so everything you'll put in your own patch folder must be available publicly.

If a feature like this is added in a 3rd party tool to help on getting non-upstream patches, it must be very clear for everyone involved, to avoid any license breach.

We strongly suggest to contribute upstream if you want to apply modifications to it: this is easier for everyone to deal with (no actual fork, centralizing contributions and so on). We are always happy to have upstream contributors and we'll help as far as we can to review and merge new features or bug fixes.

ixs commented 1 year ago

Thank you @olivierlambert for raising that concern. Yes, every modification to an AGPLv3 licensed source code also needs to be made available to the users of the program built from that source code.

I think this is very sensible and I also would expect every user of such software to comply with the licenses.

As such I am not sure what more is needed because I believe the license is already very clear. Would a reminder to that effect in the README.md be better? Personally I think that is just redundancy, but I also have no strong feelings about it either way.

olivierlambert commented 1 year ago

It's just that having an dedicated mechanism to allow running specific patches is almost encouraging forking/modifying the project outside the first reflex that should be "upstream first" (even outside the purely "legal" aspect of the license).

Do we really want to push users to do that? (it's an open question).

ronivay commented 1 year ago

Hi,

I kinda like the idea of such feature. Then again i can't think of many things you'd need to patch like this that couldn't be contributed to upstream. I unfortunately see this feature being mostly used to remove "no support" banners and such which i don't like. There already is a way to use custom repo(s) for XO and plugins which imo is the way to go if fork is needed. Like that it's way more obvious that you need to consider the license as well (when making a fork).

olivierlambert commented 1 year ago

It's still unclear from OP about the kind of patches he had in mind :thinking: Some examples might be interesting to share to see if it can really brings extra value vs pushing upstream (but I have hard time to see what could do that).

ixs commented 1 year ago

It's just that having an dedicated mechanism to allow running specific patches is almost encouraging forking/modifying the project outside the first reflex that should be "upstream first" (even outside the purely "legal" aspect of the license).

Do we really want to push users to do that? (it's an open question).

If I were to summarize this, I'd see two points:

That assumes that all patches are useful to be upstreamed. I do not believe so.

As for me, I have two XO instances at home. What I mainly use the patch functionality for is to change the colors of the interface to easily keep them apart, saving me from looking at the URL. Sure, we could add theming support upstream, but developing upstreamable theming support is more work than I would be interested in. Writing a quick plugin is something I can do but for more I do not know enough Javascript. A decent skinning interface I would feel good enough to actually submit upstream? No idea where to even start. But a quick HTML or CSS patch? Sure, that's easy.

I kinda like the idea of such feature. Then again i can't think of many things you'd need to patch like this that couldn't be contributed to upstream. I unfortunately see this feature being mostly used to remove "no support" banners and such which i don't like. There already is a way to use custom repo(s) for XO and plugins which imo is the way to go if fork is needed. Like that it's way more obvious that you need to consider the license as well (when making a fork).

As an example, when I initially was looking at xo I wasn't that happy with the choice of redis as kv store because back then I had no experience at all with redis. So I looked how hard it would be to have XO support my existing MySQL server. I started doing the work as a coding excercise in Javascript but abandoned it eventually... But what came out of it was a plugin that allows to store users in a mysql database rather than the local redis store. THAT code was shipped at https://github.com/ixs/xo-server-auth-mysql/ and probably has exactly one user.

Using that Plugin is easy to do thanks to the 3rd party plugin support. But the CSS patch I mentioned above? Not so easy to do as a plugin but trivial as a patch.

The issue of removing "no support" banners is something I kinda feel like a red herring. There are scripts around on the internet that remove the no support banner using sed. Sure, if someone feels the need to do that, they are free to do so as long as they publish the source code. Personally I wouldn't even bother doing the work because you probably need to update patches like that all the time because the underlying html changed. If you're using xo on your personal homelab? Why bother with the extra effort, but if you wanna do it, sure... It is legal after all because the only user has access to the source. If you're a commercial enterprise using XO to offer a xen-hosting service? I think you'd be insane to do that without the support of the developer. But if you're insane enough of doing that I am pretty sure the effort to remove the "no support" banner is no blocker and neither is the inability of the current xo-install.sh script to do that for you.

So at the end of the day, I feel the generic support for local patches is going to be more useful than for just removing a banner. Sure, banners can be removed, but that can also be achieved using multiple other options. Patches are more useful during development testing or simple visual changes like my CSS hack I described above.

olivierlambert commented 1 year ago

The problem with this logic (don't tell anything to upstream and do your stuff on your own) is simply undermining the upstream: less feedback, less ideas.

About themes: luckily, we heard people asking fo it so it's planned for XO6. But if people only did things on their own side, it's just a waste of time and resources.

For Redis: nobody ever asked and wasn't aware about some initiative. It is a bummer nobody told us. We could have helped.

Same for any future idea. Please ask, explain, share your ideas, don't try to reinvent the wheel. This is core of open source: collaboration! We never refused to hear any idea from anyone. We are happy to help and move forware. XO is what it is today thanks to this feedback and common thinking.

ronivay commented 1 year ago

Yeah i guess this pretty much sums it up. Things that could be useful to others won’t get enough attention they might deserve. Not saying this feature would necessarily make it worse but would definitely make it easier.

I don’t think i want to encourage applying local patches here. It is anyway very trivial to do separately if one wants to take that route.

Anyway i appreciate the effort and suggestion. Thank you.