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

Add prebuilt devcontainer #369

Merged
merged 10 commits into from
Sep 7, 2023
Merged

Conversation

jgiannuzzi
Copy link
Member

@jgiannuzzi jgiannuzzi commented Sep 5, 2023

This PR adds a prebuilt devcontainer that allows contributors to be up and running immediately. It contains all the prebuilt C++ dependencies for the chosen architecture (both x64 and arm64 are supported), as well as the nuget dependencies. This means that this devcontainer can even be used offline or in a network-isolated environment.

Tips for the reviewers

I'd recommend looking at the changes commit by commit. The commit messages should give you a good idea of what I'm going for.

You can test the result by checking out the branch, changing the image reference in .devcontainer/devcontainer.json to ghcr.io/jgiannuzzi/parquetsharp/devcontainer:latest and opening the devcontainer in VS Code with the Dev Containers extension.

This is necessary to be able to build ParquetSharpNative offline with
a populated vcpkg cache.
@jgiannuzzi jgiannuzzi marked this pull request as draft September 5, 2023 18:02
@jgiannuzzi jgiannuzzi force-pushed the devcontainer branch 4 times, most recently from d8ada9c to 4e4ac68 Compare September 6, 2023 08:42
@jgiannuzzi jgiannuzzi marked this pull request as ready for review September 6, 2023 08:47
If we reference a baseline that is too recent, some CI runner images
will not have it. With this step we ensure it won't be the case.

This used to be done via vcpkg-configuration.json, but we couldn't rely
on it for offline devcontainer usage.
Format via Powershell VS Code extension and fix
linting issues.
Copy link
Contributor

@adamreeve adamreeve left a comment

Choose a reason for hiding this comment

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

This looks great thanks Jonathan. The "Building" section of the README should probably be updated though to mention that you can use the dev container extension in visual studio code

Edit: I originally requested changes but have changed to approved as this doesn't need to block the PR being merged, it could be done later

mfkl
mfkl previously approved these changes Sep 7, 2023
Copy link
Contributor

@mfkl mfkl left a comment

Choose a reason for hiding this comment

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

My parquet knowledge is rather limited, but that was very easy to build the whole thing (took a while though) 👍

log
-- Configuring done
-- Generating done
-- Build files have been written to: /workspaces/ParquetSharp/build/x64-linux-release
[  4%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/Buffer.cpp.o
[  4%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/BufferReader.cpp.o
[  8%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/BufferOutputStream.cpp.o
[ 11%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/ColumnChunkMetaData.cpp.o
[ 15%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/ColumnDecryptionProperties.cpp.o
[ 15%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/ColumnDecryptionPropertiesBuilder.cpp.o
[ 15%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/ColumnCryptoMetaData.cpp.o
[ 20%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/ColumnDescriptor.cpp.o
[ 28%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/ColumnReader.cpp.o
[ 35%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/ColumnEncryptionProperties.cpp.o
[ 35%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/ColumnPath.cpp.o
[ 35%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/Enums.cpp.o
[ 35%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/ColumnEncryptionPropertiesBuilder.cpp.o
[ 35%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/ColumnWriter.cpp.o
[ 35%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/ExceptionInfo.cpp.o
[ 37%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/FileDecryptionProperties.cpp.o
[ 42%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/FileDecryptionPropertiesBuilder.cpp.o
[ 42%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/FileEncryptionPropertiesBuilder.cpp.o
[ 46%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/FileEncryptionProperties.cpp.o
[ 46%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/FileMetaData.cpp.o
[ 48%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/GroupNode.cpp.o
[ 51%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/KeyValueMetadata.cpp.o
[ 55%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/ManagedOutputStream.cpp.o
[ 57%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/LogicalType.cpp.o
[ 60%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/ManagedRandomAccessFile.cpp.o
[ 60%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/MemoryPool.cpp.o
[ 60%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/Node.cpp.o
[ 64%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/OutputStream.cpp.o
[ 64%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/ParquetFileReader.cpp.o
[ 71%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/ParquetFileWriter.cpp.o
[ 73%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/PrimitiveNode.cpp.o
[ 77%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/RandomAccessFile.cpp.o
[ 77%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/ReaderProperties.cpp.o
[ 77%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/ResizableBuffer.cpp.o
[ 80%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/RowGroupMetaData.cpp.o
[ 84%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/RowGroupReader.cpp.o
[ 84%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/RowGroupWriter.cpp.o
[ 88%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/SchemaDescriptor.cpp.o
[ 88%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/Statistics.cpp.o
[ 88%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/TypedColumnReader.cpp.o
[ 95%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/TypedColumnWriter.cpp.o
[ 95%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/TypedStatistics.cpp.o
[ 95%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/WriterProperties.cpp.o
[ 97%] Building CXX object cpp/CMakeFiles/ParquetSharpNative.dir/WriterPropertiesBuilder.cpp.o
[100%] Linking CXX shared library /workspaces/ParquetSharp/bin/x64-linux/ParquetSharpNative.so
[100%] Built target ParquetSharpNative
root ➜ /workspaces/ParquetSharp (devcontainer) $ 

I'm wondering whether it would make sense to have the main CI script use that docker image too?

adamreeve
adamreeve previously approved these changes Sep 7, 2023
@jgiannuzzi jgiannuzzi dismissed stale reviews from adamreeve and mfkl via 05687c1 September 7, 2023 12:24
@jgiannuzzi
Copy link
Member Author

The "Building" section of the README should probably be updated though to mention that you can use the dev container extension in visual studio code

Great idea Adam! I updated the documentation in 7a3631d.

@jgiannuzzi
Copy link
Member Author

My parquet knowledge is rather limited, but that was very easy to build the whole thing (took a while though) 👍

Imagine when you need to also build Arrow and its 80+ dependencies... Even on a beefy machine, it feels like it's taking forever 😅

I'm wondering whether it would make sense to have the main CI script use that docker image too?

It's an interesting idea, but we actually need to build under a CentOS 7 container for maximum backwards compatibility. And before you ask, a dev container image based on CentOS 7 would be very hard to build and probably quite inconvenient, as it's ancient.

@jgiannuzzi jgiannuzzi merged commit ea865a4 into G-Research:master Sep 7, 2023
29 checks passed
@jgiannuzzi jgiannuzzi deleted the devcontainer branch September 7, 2023 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants