Evaluate GitHub Actions for CI #1
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5ec5f83 to
00c6dc0
Compare
One to build with varying OS and JDK but the version of PostgreSQL preinstalled in the runner environment, and one that also builds PostgreSQL from source (or will, when completed).
5842454 to
6a53a99
Compare
For this workflow, using the runner-supplied PG, it can just be pg_config and rely on the PATH. Which would be the default if it were not supplied, but more of the script would have to be touched to remove uses of the variable than just to set it to pg_config.
The preceding commit clearly showed that sudo -E did what it says on the tin, preserved the environment, JAVA_HOME was pointing to 9 in the sudo environment, and sudo -E java still ran 8. Huh? Anyway, with this working, expand the matrix a little.
6a53a99 to
7ee89a6
Compare
In this particular workflow (relying on the PostgreSQL preinstalled on the runner), this step won't work, because the preinstalled PG was built with MSVC. But this gets the pieces in place. It would work, in a workflow where the PostgreSQL was built by MinGW; it would even work in this workflow, if we give pljava-so a set of rules for building with MinGW against an MSVC-built PostgreSQL.
The Windows runner seems to have an extra pg_config somewhere on the path, that reports it was built with MinGW and installed in paths containing Strawberry that don't really exist. It has cropped up for others too in other settings: timescale/timescaledb#555 The pg_config found in $PGBIN refers to a PostgreSQL that was built with MSVC, and its reported directories really exist. Specify that one explicitly when running on Windows. In passing, avoid trying to run the jshell install-and-test with Java 9 in Windows (it will wait forever for someone to type on the console).
Using the bash environment in Windows offers the convenience of one scripting language to tee up the install-and-test, but at the cost of getting paths we need (jdbcJar and mavenRepo) munged into Unix style. They have to be unmunged, as the Windows native Java can't use them that way.
It can't work in this workflow, where the supplied PostgreSQL was built with MSVC, at least for now. Re-enable the Linux and Mac builds to make sure the MinGW changes didn't break them.
Structure the matrix in each the same as in ci-runnerpg.yml (except that both of these will have a choice of PostgreSQL version, rather than being stuck with the version supplied on the runner). Both of these need to be completed, of course. There is much duplication among these three workflows (pretty much everything after installing or building PostgreSQL). They will probably be easier to get working that way. After most of the tricky bits have been worked out, it may be possible to combine them all into one giant uberworkflow with a lot of conditional steps. That feels like premature optimization for now.
The dfa() method has been renamed stateMachine() in master.
Now that 1.6.0 is released, the next release should include an extension SQL file allowing upgrade from 1.6.0. Advance version to 1.6-SNAPSHOT. This is a change from past practice, which always updated to 1.x.<next micro version>-SNAPSHOT, which always created a change in every POM that would have to be resolved out when merging up to master. This way will require the same resolution tedium once, but not for future 1.6.x releases. 1.6-SNAPSHOT will just always mean a snapshot on the way to whatever will be released next in the 1.6 line.
Now that there will be more than one repeatable annotation type, and there isn't really anything else about the containing annotation that matters. In passing, squelch a deprecation warning in LexicalsTest (a junit method was deprecated, in favor of one from hamcrest).
Introduce a Repeatable subclass of AbstractAnnotationImpl that will carry references to its source Element and AnnotationMirror so they are available a subclass's characterize method, if any.
In passing, give FunctionImpl's appendNameAndParams an extra boolean parameter to suppress parameter names, so it can be used generating CREATE CAST, where function parameter names aren't wanted.
In passing, remove its postgresql_ge_80300 conditionals. The exact oldest PostgreSQL version supportable by PL/Java 1.6 might still be slightly negotiable, but definitely won't be that far back.
Routine tests should include sqlj.remove_jar with undeploy=true, as those actions also come from the SQL generator.
The SQLAction annotation itself can now simply be repeated.
Drop the extra indentation left behind now that @SQLAction needn't be nested in @SQLActions.
Should have been removed in a0efb99.
PL/Java's class loader has historically handled resource lookups by returning a URL that has a custom URLStreamHandler wired in. Permission to do that wasn't included in the pljava.policy shipped with 1.6.0. Fix that. Addresses tada#322. Still does not get closer to fixing tada#266, but at least avoids making it worse.
Now that 1.5.7 is released, the next release should include an extension SQL file allowing upgrade from 1.5.7.
The breaking-out of pre-1.6 release notes in the 1_6 branch is not handled perfectly by git; it recognizes that the pre-1_6 file began as a copy of the original, but it doesn't infer that changes merging up from 1_5 should be put there, leaving a small chore during conflict resolution. I suppose that may happen every time, but not be too burdensome.
Now that 1.6.1 is released, the next release should include an extension SQL file allowing upgrade from 1.6.1.
Although XSLT 1.0 is very much like punishment when a modern version is available with Saxon, it does have the advantage of being included in Java rather than a large separate download. Now that it's usable, the example functions may as well be polished a bit (say, by making the uninteresting-but-for-testing 'how' parameters optional, and accepting an 'adjust' parameter) and made suitable for day-to-day use. One useful feature of the XSLT 1.0 transformer is the ability to indent XML: great for readability, not available in core PostgreSQL, and easy to do here. It doesn't require any particular transformation defined; the default identity transform from the no-argument TransformerFactory.newTransformer is enough. So make it possible to pass null for transformName for a plain identity transform, and add optional indent and indentWidth direct arguments to make it simple. This can also serve as an example to clarify just how one gets xalan to indent, as the details are subtle enough to have needed hashing out on Stack Overflow [1]. [1] https://stackoverflow.com/a/60610218/4062350
The ability in xalan to call out to Java methods can be extremely useful in the otherwise very limited XSLT 1.0 dialect. But it sorely needs an example, being fiddly enough to get right the first time that newcomers might otherwise flee in frustration.
and add a regression test. Addresses tada#330.
It's tedious to duplicate them in an @SQLAction GRANT or REVOKE. Maybe that indicates it would be useful to add grant/revoke support in annotations someday.
Have to access the value directly for internal purposes, as GetConfigOption will honor the GUC_SUPERUSER_ONLY on it. Add tests as non-superuser to the Travis and AppVeyor configs. Not having any was an oversight. In passing, try adding PG 13 and Java 15 to the Travis and AppVeyor builds. I have not otherwise checked whether those are available yet in those services. This should be a way to find out.
It appears that the time is not yet ripe.
The one failed Travis build appears to have been a Travis issue.
The system property org.postgresql.pljava.policy.trial, if supplied, is a URI naming another policy file that can serve as a second chance for accesses denied by the real policy laid out by pljava.policy_urls. If the denied access is allowed by this policy, it is allowed, but logged, so needed permissions can be identified while running code. If denied by this policy, the denial is final. A good bit of honest work in this policy goes to abbreviating the log output in a useful way. The approach is to keep one stack frame above and one below each crossing of a module or protection-domain boundary, with ... replacing intermediate frames within the same module/domain, and the code source/principals of the failing domain shown at the appropriate position in the trace.
Java has an AllPermission, which can be granted in a TrialPolicy in order to identify, log, and allow any and all requested permissions that are not granted in the normal policy. Some may not be comfortable proceeding that way, and because Java does not have negative permissions, there is no way to say "grant AllPermission except for these that I would like to exclude". Therefore, org.postgresql.pljava.policy.TrialPolicy$Permission is provided here, as pretty much exactly "AllPermission except for a couple dozen unsurprising exclusions." This permission can be granted in a TrialPolicy (and, in the unlikely event that any of the permissions it excludes also have to be granted there, they can be granted explicitly, in the usual way). A package in a named module that implements any new Permission must be exported to java.base; this is moved to the new package org.postgresql.pljava.policy and so exported.
The provider() method on a service provider has to be static. Misdeclaring this one never broke anything, as the service loader simply fell back on the constructor, but later service loader passes after the policy gets installed would cause false-alarm log entries from the constructor.
There isn't a perfect method available (yet) to inquire "could I doInPG() or log() at this moment without blocking?", so this check may unnecessarily send the message off to System.err on occasions when log() would have worked fine. But it's better than blocking. In passing, add a permission to read another property that the Java runtime started using in Java 14 but forgot to give itself permission to read. It's at the false-alarm level of severity, as the runtime behaves gracefully when unable to read the property, but again, if using TrialPolicy, it kind of spams the log.
Such a build appears to work, and is listed as PG 13, but on closer inspection turns out to have built with 12 anyway, which apparently is what's in pacman.
Factoring out some of the common elements of the Travis and AppVeyor (and, one day, GitHub Actions) scripts can be a project for another day.
CI logs indicate some PG 13 builds, anyway, are giving -Wimplicit-fallthrough=3 to gcc. The gcc docs say with =3, a /*FALLTHROUGH*/ comment should be recognized. Warnings are also being generated for a switch in Type.c where they should not be: elog(ERROR is already marked with pg_unreachable in elog.h. I wonder whether adding an explicit pg_unreachable will work any better.
Not strictly related to the fallthrough business, but of the general nature of lint, and hardly worth a pull request of its own.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce a GitHub Actions workflow that might be expanded to a functioning CI.