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

postgresql16*: new ports #20205

Merged
merged 11 commits into from
Sep 16, 2023
Merged

postgresql16*: new ports #20205

merged 11 commits into from
Sep 16, 2023

Conversation

dgilman
Copy link
Contributor

@dgilman dgilman commented Aug 28, 2023

Description

The most important change here is that I have changed ownership from @jyrkiwahlstedt and made the port openmaintainer. He has been busy elsewhere for a while and hasn't been on GitHub in some time so I think the change is justified. It is also a new port anyway.

This is a beta release, when the official one comes out I'd rename the port to postgresql16.

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • referenced existing tickets on Trac with full URL?
  • checked your Portfile with port lint --nitpick?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?
  • checked that the Portfile's most important variants haven't been broken?

@reneeotten
Copy link
Contributor

can we just wait for the official release - any idea on when that will be?

@dgilman
Copy link
Contributor Author

dgilman commented Aug 29, 2023

It will be released some time in the next month or two. I think there have been beta versions in MacPorts before, though, because the portfile had a livecheck regex commented out that only matches pre-release versions.

@reneeotten
Copy link
Contributor

It will be released some time in the next month or two. I think there have been beta versions in MacPorts before, though, because the portfile had a livecheck regex commented out that only matches pre-release versions.

I am not sure, not seeing any previous -devel ports with a quick look, but who knows. I personally don't really care about it, but regardless the CI fails for the "-doc" port so that should be fixed. Then we can see what others think about it.

@dgilman
Copy link
Contributor Author

dgilman commented Aug 29, 2023

It looks like Jyrki went from \d\dbeta\d to \d\d[.]\d without additional -dev ports. So that option is also on the table. f4a7e25

@barracuda156
Copy link
Contributor

@dgilman @reneeotten Please allow me to test prior to merging.

@reneeotten
Copy link
Contributor

It looks like Jyrki went from \d\dbeta\d to \d\d[.]\d without additional -dev ports. So that option is also on the table. f4a7e25

Yeah, that would avoid adding a new port and doing the "replaced_by" stuff as just renaming a port cannot be done. Only drawback in that approach is likely that one would need to increase the "epoch" as I think that version "16" would not be considered newer than, for example, "16rc2". But that should be tested to make sure.

@barracuda156
Copy link
Contributor

@dgilman @reneeotten To begin with, we need legacysupport:

In file included from ../../src/include/pgstat.h:15,
                 from controldata_utils.c:38:
../../src/include/portability/instr_time.h: In function ‘pg_clock_gettime_ns’:
../../src/include/portability/instr_time.h:116: warning: implicit declaration of function ‘clock_gettime’
../../src/include/portability/instr_time.h:116: error: ‘CLOCK_REALTIME’ undeclared (first use in this function)
../../src/include/portability/instr_time.h:116: error: (Each undeclared identifier is reported only once
../../src/include/portability/instr_time.h:116: error: for each function it appears in.)

@barracuda156
Copy link
Contributor

Then, something is broken here:

/usr/bin/gcc-4.2 -std=gnu99 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -pipe -Os -arch ppc -I../../../src/include  -isystem/opt/local/include/LegacySupport -I/opt/local/include -I/opt/local/include/libxml2  -I/opt/local/include  -c -o vacuum.o vacuum.c
/usr/bin/gcc-4.2 -std=gnu99 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -pipe -Os -arch ppc -I../../../src/include  -isystem/opt/local/include/LegacySupport -I/opt/local/include -I/opt/local/include/libxml2  -I/opt/local/include  -c -o vacuumparallel.o vacuumparallel.c
{standard input}:605:Parameter syntax error (parameter 3)
{standard input}:615:Parameter syntax error (parameter 3)
{standard input}:1030:Parameter syntax error (parameter 3)
gnumake[3]: *** [vacuumparallel.o] Error 1
gnumake[3]: *** Waiting for unfinished jobs....
{standard input}:185:Parameter syntax error (parameter 3)
gnumake[3]: *** [vacuum.o] Error 1

Let me look into this.

@barracuda156
Copy link
Contributor

I think I have found broken code. Let me make a patch and try.

@barracuda156
Copy link
Contributor

@dgilman @reneeotten Turned out, it was broken on purpose in postgres/postgres@8ded656

Anyway, this patch fixes the build (legacysupport PG still needed, since upstream has removed the check for clock_gettime):
patch-fix-ppc-asm.diff.zip

@dgilman
Copy link
Contributor Author

dgilman commented Aug 30, 2023

The commit message mentions that modern assemblers can emit compatible instructions. Is it possible to blacklist old compilers instead of having to try and carry this patch forever?

@barracuda156
Copy link
Contributor

barracuda156 commented Aug 30, 2023

@dgilman The problem here is that even if the comment is accurate (which we cannot be sure of), we have no selection of assemblers for macOS PowerPC. Using the latest GCC won’t help, it does not provide as.

From the looks of things upstream did a wrong thing removing the code.

It is possible that the code can be improved and made more concise. Maybe lwsync can be supported via ppc intrinsics (just a guess here). But I can’t deal with this in a couple of hours.

Be sure, I do not want to carry a huge patch forever myself (given that pretty likely I will have to test it and fix with subsequent updates). Solutions – either bring this to attention of upstream that their assumptions were wrong and consequently the move was wrong either, and should be reverted; or rewrite the code to make it more portable along releases.
Writing a brand new assembler for old PPC, I am afraid, is out of scope of being feasible.

P. S. If you wish, you may add me as a co-maintainer for PowerPC implementation, and I will take care of the patch as long as that is needed – until, hopefully, it is fixed in a better way.

@barracuda156
Copy link
Contributor

Before I bothered to check commit history and find the breakage, I have already figured out to replace lwsync with either isync or sync. What I missed is that the new code uses invalid form of lwarx which smuggles in an extra argument.

As can be seen, the instruction takes 3 arguments: https://opensource.apple.com/source/xnu/xnu-792.6.76/osfmk/ppc/commpage/atomic.s.auto.html
While the code now passes 4. Of course this cannot work.

I suggest we merge what we have for now (just restoring a working upstream code), and later I will see if a) we can make lwsync work (it is mentioned in intrinsic header) and what will be a better way to fix broken lwarx/ldarx.

@barracuda156
Copy link
Contributor

For the legacysupport, it is worth specifying the latest Darwin version which needs it. Check the portgroup. (It is not in the portfile now, or did I just overlook it?)

Copy link
Contributor

@ryandesign ryandesign left a comment

Choose a reason for hiding this comment

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

In my view, there are two separate port naming strategies that can be employed:

  1. foo and foo-devel. In this case, foo is the latest stable version, foo-devel is the latest development version, and they intentionally install files to the same locations and conflict with one another and the devel port is a drop-in replacement for the non-devel port. Examples: libpixman, cairo, pango, glib2, graphviz, minivmac.
  2. bar1, bar2, bar3. In this case, each port provides the latest version of the specified major version of the software, they intentionally install files to different locations and do not conflict with one another. There is also a bar_select port to let you select which version of bar you want to use at the command line with the standard name without having to specify the version number. Examples: clang and llvm, gcc, php, perl5*, python**.

*perl5 is unique in that it does not have a working perl_select port. It predates the existence of port select so it's not a good model for what should be done today.

**python is unique in that Josh has chosen to use the -devel suffix on development versions of python for which there is not yet any stable version, such as python312-devel today. When the stable version comes around, Josh will copy the python312-devel port to python312, making adjustments along the way, and hopefully leaving the stub python312-devel port marked replaced_by python312 for awhile. This is a lot of extra work that I view as unnecessary for this case, hence the manner in which I handle the php ports, for example php83 today is 8.3.0beta3 and will eventually become 8.3.0 final (with the corresponding necessary epoch increase).

If you wish to use the devel naming scheme for postgresql16, then the suffix must be -devel not -dev.

Many of the comments I made below may apply to previous postgresql ports as well. All of the postgresql ports should be kept as similar as possible. This has, to a great degree, not happened with these ports so far, so straightening that out and figuring out which differences between them are intentional and which are not and should be back- or forward-ported to the other ones may be a lot of work.

databases/postgresql16-dev/Portfile Outdated Show resolved Hide resolved
databases/postgresql16-dev/Portfile Outdated Show resolved Hide resolved
databases/postgresql16-dev/Portfile Outdated Show resolved Hide resolved
databases/postgresql16-dev/Portfile Outdated Show resolved Hide resolved
databases/postgresql16-dev/Portfile Outdated Show resolved Hide resolved
databases/postgresql16-server/Portfile Outdated Show resolved Hide resolved
@dgilman
Copy link
Contributor Author

dgilman commented Sep 10, 2023

Done:

  • added barracuda as maintainer per comment above
  • @barracuda156 are https://trac.macports.org/ticket/66689 and https://trac.macports.org/ticket/67203 done? if so, i will close here. it seems like we are ICU everywhere and PPC is building on your machine
  • update to version 16rc1
  • switched port name to just postgresql16. In the past the port (without suffix) went from beta -> rc -> final release, and that's probably what the end user would expect from a postgresql16 port anyway.
  • addressed review feedback
  • added many Closes: to commit messages

User homedir / shell changes:
There are a few issues ( https://trac.macports.org/ticket/24180 https://trac.macports.org/ticket/14083 https://trac.macports.org/ticket/64286 ) that have to do with a poor initial choice of shell and homedir for the postgresql user in the -server port. The add_users Tcl function doesn't really support migrating account properties after a user account has been set up. I tried throwing system "dscl ..." into the Portfile, it fails because of some sandboxing (?) that denies dscl access to change the properties.

Barring some trick to get around the sandbox that I don't know about, I think the way forward here is to just set the defaults right in postgresql16 and let the fixes trickle out over time. They aren't breaking any fundamental functionality with the port.

Backporting changes:
I didn't do any backporting of changes to earlier PG releases in this PR. However, I am thinking the way forward is to mark 11 and before with the depreciated portgroup and only copy changes to 12+. 11 goes out of support in November.

TODO:

@dgilman dgilman force-pushed the postgresql16 branch 2 times, most recently from cafb1f7 to 9780997 Compare September 10, 2023 20:18
@barracuda156
Copy link
Contributor

@dgilman postgresql13 and postgresql14 should build fine now on PPC. I have just re-built postgresql13 in Rosetta to make sure. So yes, those tickets can be closed, I believe.
(Not sure if the fix has been backported to all earlier versions where it may be needed though.)

Re sandboxing, while I do not know whether it is relevant, but we use the following in some ports:

pre-test {
# test infrastructure uses /bin/ps, which is forbidden by sandboxing
append portsandbox_profile " (allow process-exec (literal \"/bin/ps\") (with no-profile))"
}

@mascguy
Copy link
Member

mascguy commented Sep 10, 2023

Only drawback in that approach is likely that one would need to increase the "epoch" as I think that version "16" would not be considered newer than, for example, "16rc2". But that should be tested to make sure.

To avoid dealing with all of that, the current version of the port could start with 15. So something like 15.99.rc1.

@mascguy mascguy changed the title postgresql16-dev: new port postgresql16*: new ports Sep 10, 2023
@dgilman dgilman mentioned this pull request Sep 16, 2023
12 tasks
@dgilman dgilman force-pushed the postgresql16 branch 2 times, most recently from 58deaa7 to 7f6925a Compare September 16, 2023 17:20
@dgilman
Copy link
Contributor Author

dgilman commented Sep 16, 2023

I've uncommented the dscl stuff. Not sure what I was running into before but it is now running correctly / offering the correct privs. I think it may have been an issue of the permission popup modal the system does not correctly firing or me accidentally denying it.

This PR is good for final review

@mascguy mascguy merged commit 4c82843 into macports:master Sep 16, 2023
3 checks passed
@ryandesign
Copy link
Contributor

Only drawback in that approach is likely that one would need to increase the "epoch" as I think that version "16" would not be considered newer than, for example, "16rc2". But that should be tested to make sure.

To avoid dealing with all of that, the current version of the port could start with 15. So something like 15.99.rc1.

The epoch exists for cases like this. It's much better to use the epoch as intended rather than lie to the user about the port's version number.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants