search

postgrespro / pgsphere

PgSphere provides spherical data types, functions, operators, and indexing for PostgreSQL.
https://pgsphere.org
BSD 3-Clause "New" or "Revised" License
16 stars 15 forks source link

Makefile cleanup #75

Closed df7cb closed 8 months ago

df7cb commented 9 months ago
esabol commented 9 months ago

@df7cb wrote:

• Remove support from upgrading from pre-1.2.0 versions

It doesn't affect our installations, but I'm not sure I agree with this in principle. What's the harm in supporting upgrading from pre-1.2.0 versions?

df7cb commented 9 months ago

Look at the mess of files in there, and the code in the Makefile that was used to support the pre-1.2 mess. This is utterly painful to look at, and will likely drive away some people that might have conrtibuted to pgsphere otherwise. It took me several attempts over several months of looking at this to figure out how it actually works, and I do know how to read PG extension Makefiles.

https://github.com/postgrespro/pgsphere/pull/75/commits/8da3fee56ada97e2537d65651d95b013eff96a02#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52 (this commit) might still look okayish, but if you put https://github.com/postgrespro/pgsphere/pull/75/commits/e2bfda15061f0bb9bff9adf3029d0a666861c561 next to it, half of the code in the old Makefile was dedicated to the old stuff which is very likely not used anywhere anyway.

I don't fancy removing support for old versions, but in this case, I think it does help a lot.

@msdemlei, could you chime in here?

msdemlei commented 9 months ago

On Thu, Oct 05, 2023 at 09:22:03AM +0000, Christoph Berg wrote:

I don't fancy removing support for old versions, but in this case, I think it does help a lot.

@msdemlei, could you chime in here?

I agree that the upgrade scripts are extremely painful.

You can certainly drop everything that has "gavo" in it. Having an upgrade option from unpackaged would probably be still a good thing, so I suspect an unpackaged -> 1.0 script would make sense.

On dropping upgrade scripts for released versions... Well, we just don't know what versions are still running out there. So, for those I'd rather be cautious in terms of dropping.

Before I say anything more: Is this problem particularly bad for us? If so, why? Is it because we have more releases we may have to upgrade from? Is it because our upgrade script management is particularly clumsy? Is it because others have more generic upgrade script that work between many versions thanks to tastefully applied IF EXISTS-s and friends?

df7cb commented 9 months ago

Fwiw "remove everything with gavo in it" is exactly what this patch is doing. The upgrade path from unpackaged went to some gavo version, then through some more gavo versions, and then to 1.2.

The problem is worse for pgsphere than the average extension because all the .sql files are compiled from smaller bits. This is basically a good idea, but the Makefile rules for compiling the intermediate gavo (and "unpackaged) files were extremely convoluted and painful to look at. With the cleanup, the Makefile now makes sense when looking at it.

esabol commented 9 months ago

I upgraded our pgsphere installations from 1.1.something not that long ago (on the order of months), so I feel it is premature to drop support for upgrading from older versions. While I'm sure the files could be tidied up some, personally, I did not find the Makefile or the upgrade scripts to be particularly confusing when I submitted a PR a while back. I recommend adding more comments and structure to the Makefile to make it less confusing over dropping support for upgrading from older versions.

df7cb commented 9 months ago

What about the "from unpackaged" part? That's unsupported since PG13, can we remove that yet?

esabol commented 9 months ago

What about the "from unpackaged" part? That's unsupported since PG13, can we remove that yet?

Sure! I don't think anyone has objected to that (or the other changes).

df7cb commented 9 months ago

Aye, I'll rebase that part when I get back from vacation after next week.

df7cb commented 8 months ago

I rebased this branch to current master and dropped the "drop 1.2" part. I verified that upgrading from 1.0 (Debian package version 1.1.1+2018.10.13-1 [*]) works (alter extension update and manually running the tests on that installation). No problems found with these changes.

There is a problem with the spoint3 change from #74 (the opclass from 1.0 already contained the function at that time), but that problem is independent from this PR; this PR is good to be merged as far I am concerned. I'll address the problem separately.

[*] The repo doesn't contain any tags from before 1.1.5, so I used that version instead

df7cb commented 8 months ago

Fwiw if we want the git history to look pretty, I can rebase each of the other PRs after one of then has been merged.

vitcpp commented 8 months ago

@df7cb Thank you for the patch! I think we can merge it. I propose to merge this patch first.

About tags. I restored the tags in this repo down to 1.1.5 but I was not able to identify commits of older versions (issue #9). If you have some ideas how to find commits for older tags, please, let me know.

vitcpp commented 8 months ago

@df7cb One more forgotten thing... Could you please squash the commits? I use the merge option to put PR into the master. It preserves the authorship. I'm not sure about preserving of the authorship when I squash the commits using github. Thank you!

df7cb commented 8 months ago

@vitcpp the changes are logically separated, so I would recommend not to squash them. (In any case, GitHub preserves the commit authors.)