Skip to content

Windows support #13

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Apr 22, 2016
Merged

Windows support #13

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d617516
* added windows support
Apr 19, 2016
263d5ed
* external tar not needed for tests
MarekMalevic Apr 20, 2016
01b43bf
* win support fix
MarekMalevic Apr 20, 2016
7ce238d
* windows support libraries added
Apr 20, 2016
6472d0f
* findbug hint fix
MarekMalevic Apr 20, 2016
53686d0
Merge remote-tracking branch 'origin/windows-support' into windows-su…
MarekMalevic Apr 20, 2016
85f8564
* style fix
MarekMalevic Apr 20, 2016
9378e7c
* style fix
MarekMalevic Apr 20, 2016
d4ac576
* reverted system tar usage for non-windows os
Apr 21, 2016
98b3297
* extraction improved
MarekMalevic Apr 21, 2016
5c6804a
* style fix
Apr 21, 2016
ffb7ffd
* PR fixes
Apr 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,12 @@
<artifactId>commons-lang3</artifactId>
</dependency>

<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-compress</artifactId>
<version>1.11</version>
</dependency>

<dependency>
<groupId>commons-io</groupId>
<artifactId>commons-io</artifactId>
Expand Down
19 changes: 19 additions & 0 deletions repack-postgres.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@ cd `dirname $0`
PACKDIR=$(mktemp -d -t wat.XXXXXX)
LINUX_DIST=dist/postgresql-$VERSION-linux-x64-binaries.tar.gz
OSX_DIST=dist/postgresql-$VERSION-osx-binaries.zip
WINDOWS_DIST=dist/postgresql-$VERSION-win-binaries.zip

mkdir -p dist/ target/generated-resources/
[ -e $LINUX_DIST ] || wget -O $LINUX_DIST "http://get.enterprisedb.com/postgresql/postgresql-$VERSION-linux-x64-binaries.tar.gz"
[ -e $OSX_DIST ] || wget -O $OSX_DIST "http://get.enterprisedb.com/postgresql/postgresql-$VERSION-osx-binaries.zip"
[ -e $WINDOWS_DIST ] || wget -O $WINDOWS_DIST "http://get.enterprisedb.com/postgresql/postgresql-$VERSION-windows-x64-binaries.zip"

tar xzf $LINUX_DIST -C $PACKDIR
pushd $PACKDIR/pgsql
Expand Down Expand Up @@ -38,4 +40,21 @@ tar cjf $OLDPWD/target/generated-resources/postgresql-Darwin-x86_64.tbz \
bin/postgres
popd

rm -fr $PACKDIR && mkdir -p $PACKDIR

unzip -q -d $PACKDIR $WINDOWS_DIST
pushd $PACKDIR/pgsql
tar cjf $OLDPWD/target/generated-resources/postgresql-Windows-x86_64.tbz \
share \
lib/iconv.lib \
lib/libxml2.lib \
lib/ssleay32.lib \
lib/ssleay32MD.lib \
lib/*.dll \
bin/initdb.exe \
bin/pg_ctl.exe \
bin/postgres.exe \
bin/*.dll
popd

rm -rf $PACKDIR
143 changes: 116 additions & 27 deletions src/main/java/com/opentable/db/postgres/embedded/EmbeddedPostgres.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,41 @@
*/
package com.opentable.db.postgres.embedded;

import static com.google.common.base.MoreObjects.firstNonNull;
import com.google.common.base.Preconditions;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.io.Closeables;
import com.google.common.io.Files;
import org.apache.commons.codec.binary.Hex;
import org.apache.commons.compress.archivers.tar.TarArchiveEntry;
import org.apache.commons.compress.archivers.tar.TarArchiveInputStream;
import org.apache.commons.compress.compressors.bzip2.BZip2CompressorInputStream;
import org.apache.commons.io.FileUtils;
import org.apache.commons.io.IOUtils;
import org.apache.commons.lang3.SystemUtils;
import org.apache.commons.lang3.time.StopWatch;
import org.postgresql.ds.PGSimpleDataSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.sql.DataSource;
import java.io.BufferedInputStream;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.Closeable;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.net.ServerSocket;
import java.net.UnknownHostException;
import java.nio.channels.FileLock;
import java.nio.channels.OverlappingFileLockException;
import java.nio.file.FileSystems;
import java.nio.file.Path;
import java.security.DigestInputStream;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
Expand All @@ -42,25 +66,9 @@
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

import javax.sql.DataSource;

import com.google.common.base.Preconditions;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.io.Closeables;
import com.google.common.io.Files;

import org.apache.commons.codec.binary.Hex;
import org.apache.commons.io.FileUtils;
import org.apache.commons.io.IOUtils;
import org.apache.commons.lang3.time.StopWatch;
import org.postgresql.ds.PGSimpleDataSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import static com.google.common.base.MoreObjects.firstNonNull;

public class EmbeddedPostgres implements Closeable
public class EmbeddedPostgres implements AutoCloseable, Closeable
Copy link
Contributor

Choose a reason for hiding this comment

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

Closeable extends AutoCloseable so this declaration is redundant, yeah?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing

{
private static final Logger LOG = LoggerFactory.getLogger(EmbeddedPostgres.class);
private static final String JDBC_FORMAT = "jdbc:postgresql://localhost:%s/%s?user=%s";
Expand Down Expand Up @@ -342,7 +350,8 @@ private void cleanOldDataDirectories(File parentDirectory)

private String pgBin(String binaryName)
{
return new File(pgDir, "bin/" + binaryName).getPath();
final String extension = SystemUtils.IS_OS_WINDOWS ? ".exe" : "";
return new File(pgDir, "bin/" + binaryName + extension).getPath();
}

public static EmbeddedPostgres start() throws IOException
Expand Down Expand Up @@ -424,7 +433,7 @@ private static List<String> system(String... command)
}
}

private void mkdirs(File dir)
private static void mkdirs(File dir)
{
Preconditions.checkState(dir.mkdirs() || (dir.isDirectory() && dir.exists()), // NOPMD
"could not create %s", dir);
Expand All @@ -433,15 +442,91 @@ private void mkdirs(File dir)
private static final AtomicReference<File> BINARY_DIR = new AtomicReference<>();
private static final Lock PREPARE_BINARIES_LOCK = new ReentrantLock();

private File prepareBinaries(PgBinaryResolver pgBinaryResolver) {
/**
* Get current operating system string. The string is used in the appropriate postgres binary name.
*
* @return Current operating system string.
*/
private static String getOS(){
return SystemUtils.IS_OS_WINDOWS ? "Windows" : system("uname", "-s").get(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this just use the appropriate SystemUtils properties to determine the OS directly now, rather than calling uname -s?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to redesign existing functionality / naming and I am not able to test what Apple returns.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will handle that :)

}

/**
* Get the machine architecture string. The string is used in the appropriate postgres binary name.
*
* @return Current machine architecture string.
*/
private static String getArchitecture(){
if (!SystemUtils.IS_OS_WINDOWS) {
return system("uname", "-m").get(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto here, should we just use the property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same here, I cannot test Apple and don't want to break it.

}

return "amd64".equals(SystemUtils.OS_ARCH) || SystemUtils.OS_ARCH == null ? "x86_64" : SystemUtils.OS_ARCH;
}

/**
* Unpack archive compressed by tar with bzip2 compression. By default system tar is used (faster). If not found, then the
* java implementation takes place.
*
* @param tbzPath The archive path.
* @param targetDir The directory to extract the content to.
*/
private static void extractTbz(final String tbzPath, final String targetDir) throws IOException {
try {
system("tar", "-x", "-f", tbzPath, "-C", targetDir);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going to use the commons-compress way of doing it, we should do so directly, and not use system tar at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I removed it but then I returned system tar, because commons-compress way does not allow to set file modes easily and I am not able to test on Mac (on Linux I was able to test and found issues with file modes along with issues with symlinks. I have no idea what may happen on Mac.).

} catch (final Exception e) {
try (
Copy link
Contributor

@stevenschlansker stevenschlansker Apr 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

What if this exception is because of a corrupt download, or truncated file, or something? It'd get swallowed. Probably should just get rid of the fallback and use commons-compress directly and remove the system calls entirely.

final FileInputStream fin = new FileInputStream(tbzPath);
final BufferedInputStream in = new BufferedInputStream(fin);
Copy link
Contributor

Choose a reason for hiding this comment

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

Files.newInputStream will open a file and buffer it in one step, saving a declaration here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced

final ByteArrayOutputStream tarOut = new ByteArrayOutputStream();
final BZip2CompressorInputStream bzIn = new BZip2CompressorInputStream(in)
) {
final byte[] buffer = new byte[4096];
int n;
while (-1 != (n = bzIn.read(buffer))) {
tarOut.write(buffer, 0, n);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the decompression done to a byte array input stream? Why not just new TarArchiveInputStream(new Bzip2CompressorInputStream(Files.newInputStream(tbzPath)))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

simplified

}

final TarArchiveInputStream tarIn = new TarArchiveInputStream(new ByteArrayInputStream(tarOut.toByteArray()));
TarArchiveEntry entry;
String individualFile;
int offset;
FileOutputStream outputFile;

while ((entry = tarIn.getNextTarEntry()) != null) {
individualFile = entry.getName();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need any protection against .. or the like in the path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... I have no idea. I have tested it on existing archives and there is no such issue.

final File fsObject = new File(targetDir + "/" + individualFile);
if (entry.isSymbolicLink()) {
final Path target = FileSystems.getDefault().getPath(entry.getLinkName());
java.nio.file.Files.createSymbolicLink(fsObject.toPath(), target);
} else if (entry.isFile()) {
final byte[] content = new byte[(int) entry.getSize()];
offset = 0;
final int read = tarIn.read(content, offset, content.length - offset);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this guaranteed to readFully or might it be a short read?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it cannot be short read in our case.

Preconditions.checkState(read != -1, "could not read %s", individualFile);
mkdirs(fsObject.getParentFile());
outputFile = new FileOutputStream(fsObject);
IOUtils.write(content, outputFile);
outputFile.close();
} else if (entry.isDirectory()) {
mkdirs(fsObject);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

} else {
    throw new UnsupportedOperationException();
}

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added exception with entry name

}

tarIn.close();
}
}
}

private static File prepareBinaries(PgBinaryResolver pgBinaryResolver) {
PREPARE_BINARIES_LOCK.lock();
try {
if(BINARY_DIR.get() != null) {
return BINARY_DIR.get();
}

String system = system("uname", "-s").get(0);
String machineHardware = system("uname", "-m").get(0);
final String system = getOS();
final String machineHardware = getArchitecture();

LOG.info("Detected a {} {} system", system, machineHardware);
File pgDir;
Expand Down Expand Up @@ -475,10 +560,10 @@ private File prepareBinaries(PgBinaryResolver pgBinaryResolver) {
try {
Preconditions.checkState(!pgDirExists.exists(), "unpack lock acquired but .exists file is present.");
LOG.info("Extracting Postgres...");
system("tar", "-x", "-f", pgTbz.getPath(), "-C", pgDir.getPath());
extractTbz(pgTbz.getPath(), pgDir.getPath());
Files.touch(pgDirExists);
} finally {
Preconditions.checkState(unpackLockFile.delete(), "could not remove lock file %s", unpackLockFile.getAbsolutePath());
} catch (Exception e) {
LOG.error(e.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling e.getMessage() destroys the stack trace.
Prefer LOG.error("while unpacking postgres", e) which presents the stack trace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

}
} else {
// the other guy is unpacking for us.
Expand All @@ -488,6 +573,10 @@ private File prepareBinaries(PgBinaryResolver pgBinaryResolver) {
}
Preconditions.checkState(pgDirExists.exists(), "Waited 60 seconds for postgres to be unpacked but it never finished!");
}
} finally {
if (unpackLockFile.exists()) {
Preconditions.checkState(unpackLockFile.delete(), "could not remove lock file %s", unpackLockFile.getAbsolutePath());
}
}
}
} catch (final IOException | NoSuchAlgorithmException e) {
Expand Down