-
-
Notifications
You must be signed in to change notification settings - Fork 183
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
Initial addition of 10.11.5 with working unit tests. #771
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left comments on the pain points from the previous PR.
mariaDB4j-core/src/main/java/ch/vorburger/mariadb4j/DBConfigurationBuilder.java
Show resolved
Hide resolved
executables.putIfAbsent(Server, () -> new File(baseDir, "bin/mysqld" + getExtension())); | ||
executables.putIfAbsent(Client, () -> new File(baseDir, "bin/mysql" + getExtension())); | ||
executables.putIfAbsent(Dump, () -> new File(baseDir, "bin/mysqldump" + getExtension())); | ||
executables.putIfAbsent(Server, () -> new File(baseDir, "bin/mariadbd" + getExtension())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we kept the original mysql names, we would have to modify the installation script and that becomes very brittle very quickly.
executables.putIfAbsent(PrintDefaults, () -> new File(baseDir, "bin/my_print_defaults" + getExtension())); | ||
executables.putIfAbsent(InstallDB, () -> { | ||
File bin = new File(baseDir, "bin/mysql_install_db" + getExtension()); | ||
File bin = new File(baseDir, "bin/mariadb-install-db" + getExtension()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we copied over the new script name into the old script name, it will reference the new binaries. Because these were not chmod +x it would fail because it could not find that executable.
Updating the new script with the old names was untenable.
</dependency> | ||
<dependency> | ||
<groupId>ch.vorburger.mariaDB4j</groupId> | ||
<artifactId>mariaDB4j-db-mac64</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per our previous conversation on the last PR, maxOS support has been dropped. I am experimenting with some virtualbox vms to avoid buying a mac machine. But for now, mac is bespoke and not easily fixed on a linux machine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm fine with this; given even this old Mac x86 10.2.11
increasingly doesn't work for new Mac users on M1/M2/M3 ARM CPUs anymore anyway, see #497 and https://github.com/MariaDB4j/MariaDB4j#how-using-existing-native-mariadb-binaries. The real solution is probably #296 anyway... maybe I'll play with that one of these days!
mysql binaries are modified in code. win32 has been replaced with winx64 from maria downloads. Security unit test has been updated to account for root user changes in 10.4.
83db6f4
to
902fda9
Compare
@TheKnowles Great to hear back from you - Thank You for re-engaging, after #710. I will try to have a closer look at this later today or tomorrow! |
@TheKnowles OK, right, now I better understand... so those would break using MariaDB4j with older MariaDB where binaries still used to be named It IS a "breaking" change (for MariaDB4j). At the very least, this probably justifies changing the version from Does anyone watch this repo and is reading along here and would have any opinion about this? Does e.g. anyone still use a current MariaDB4j v3.0.1 release but overrides the POM dependency to an OLDER MariaDB than the 10.2.11 which MariaDB4j v3.0.1 comes with? Or use a custom binary artefact built in-house? Or does everyone "just want it to work out of the box" and is fine with, even very happy about and glad for, a future new MariaDB4j release to just depend on latest MariaDB 10.11.5 and not bothered by not able to override the binaries to older versions? Then I'm happy to merge this... @mosesn @mrdziuban do you have any views pro/con about this PR? Happy to see it? Breaking anything for you internally? |
mariaDB4j/src/test/java/ch/vorburger/mariadb4j/tests/MariaDB4jSampleTutorialTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK to merge this as-is.
I just need to separately push the binaries to Maven central, then this will build, and I'll merge it.
I think this makes sense, so what I'll do is release a 3.0.2 (without this), then bump to
Done, and it's green on Linux, but CI fails on Windows... I don't have a Windows machine. @TheKnowles have you tested this locally on Windows? Do you want to have a closer look and see what that |
The reason seems to be this:
So for some reason, even though it's mostly changed from |
Yes I will investigate. I only ran the simulated winx64 start. |
@vorburger Ok, this should be good to go. There were a handful of issues:
I have built and run all unit tests passing on Windows 11. |
Cool, Thank You so much! I'll re-test this as soon as I can get to it. |
Because I already released https://repo.maven.apache.org/maven2/ch/vorburger/mariaDB4j/mariaDB4j-db-winx64/10.11.5/ (NB the This time, I'll temporarily "hack" things, and not actually deploy it just yet, but instead |
includes bumping MariaDB v10.11.5 Windows binary to 10.11.5.1; see #771 (comment).
includes bumping MariaDB v10.11.5 Windows binary to 10.11.5.1; see #771 (comment).
done in #779 (@TheKnowles please ignore the merge conflicts that caused here; I will merge the commits from your PR here under the overall PR #772).
see c6814e6 |
includes bumping MariaDB v10.11.5 Windows binary to 10.11.5.1; see #771 (comment).
includes bumping MariaDB v10.11.5 Windows binary to 10.11.5.1; see #771 (comment).
This does not work... 😭 The
Reading https://maven.apache.org/pom.html#version-order-specification, I suspect the "correct" (?) solution to this is to use a "version qualifier" and release it as a
|
That's fascinating, I did not realize a point release would not work. |
includes bumping MariaDB v10.11.5 Windows binary to 10.11.5-fix1; see #771 for details.
includes bumping MariaDB v10.11.5 Windows binary to 10.11.5-fix1; see #771 for details.
Fascinating is one way to describe the mess that Maven versions are... I just hope (not tested!) that
And with that, #772 was green, and got merged. With that, I am (finally!) closing this PR, because I squashed the 4 commits from here into 78d17f8, which is now merged to the |
Thanks for the review and merge! In retrospect, I should have done a full run on an actual Windows machine. I was surprised how different the maria releases are, I had only done the simulated unit test run from a linux machine. |
mysql binaries are modified in code.
win32 has been replaced with winx64 from maria downloads.
Security unit test has been updated to account for root user changes in 10.4.