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 SEAL_PURE_SOURCETREE option #427

Merged
merged 4 commits into from
Mar 15, 2022

Conversation

rickwebiii
Copy link

@rickwebiii rickwebiii commented Dec 3, 2021

Add an option so the SEAL build will output all files under the build directory (i.e. the cmake -B directory). This includes thirdparty binaries and sources. This improves compatibility with other build systems and allows for concurrent builds of the SEAL library in the same repository, so long as they use separate -B directories.

In particular, I found that concurrent builds would interfere with each other when attempting to clone thirdparty git repos over each other.

@ghost
Copy link

ghost commented Dec 3, 2021

CLA assistant check
All CLA requirements met.

@rickwebiii
Copy link
Author

I need to test a number of build scenarios to ensure I don't break anything. Any recommendations? I'm on an aarch64 machine.

@WeiDaiWD
Copy link
Contributor

WeiDaiWD commented Dec 6, 2021

I need to test a number of build scenarios to ensure I don't break anything. Any recommendations? I'm on an aarch64 machine.

No good way, I wish we had GitHub actions enabled. Currently we have to manually build it with different configurations to test.

This is a quite useful improvement to the build system. I'm down to smoothing the cross build experience. How about we simplify this by removing the SEAL_PURE_SOURCETREE option and make it the default behavior?

Suppose we run cmake -B builddir .... At configuration time, dependency xxx's source will be downloaded to builddir/thirdparty/xxx-src. Do we want the dependency to be built in-source of out-of-source, like into builddir/thirdparty/xxx-build?

@rickwebiii
Copy link
Author

I'm fine with this being the default. The option was to exercise more caution against breaking existing flows.

Doing a little more testing, it looks like some things get emitted into the dotnet directory on build, so I need to track those down and relocate them as well.

@WeiDaiWD
Copy link
Contributor

WeiDaiWD commented Dec 6, 2021

Are add_subdirectory(${xxx_SOURCE_DIR}/../xxx-build) necessary?

@rickwebiii
Copy link
Author

Are add_subdirectory(${xxx_SOURCE_DIR}/../xxx-build) necessary?

Since the source code lives outside the source tree at effectively an absolute path, CMake cannot infer the binary directory and we must specify their location. It is not necessary that this be the exact location; we can just place artifacts under ${THIRDPARTY_BINARY_DIR}/xxx-build, which is the same pattern used in the GTest and Benchmark targets (though, those generate source code...).

I got errors in CMake complaining about sources living outside the sourcetree without these changes.

I have tested these changes with

cmake -S . -B build \
 -DBUILD_SHARED_LIBS=OFF \
 -DCMAKE_BUILD_TYPE=Release \
 -DCMAKE_CXX_FLAGS_RELEASE="-DNDEBUG -flto -O3" \
 -DCMAKE_C_FLAGS_RELEASE="-DNDEBUG -flto -O3" \
 -DSEAL_BUILD_BENCH=ON \
 -DSEAL_BUILD_EXAMPLES=ON \
 -DSEAL_BUILD_TESTS=ON \
 -DSEAL_USE_CXX17=ON \
 -DSEAL_USE_INTRIN=ON \
 -DSEAL_USE_MSGSL=ON \
 -DSEAL_USE_ZSTD=ON \
 -DSEAL_USE_ZLIB=ON \
 -DSEAL_THROW_ON_TRANSPARENT_CIPHERTEXT=OFF \
 -DSEAL_BUILD_SEAL_C=ON \
 -DSEAL_BUILD_STATIC_SEAL_C=ON \

and the benchmarks, tests, and examples all pass.

The only gaps in testing I think are C# bindings and HEXL. I don't think I broke HEXL given it's straightforward like the rest. I probably broke C# bindings given they're MSBuild projects with a bunch of references.

@rickwebiii
Copy link
Author

@WeiDaiWD Circling back on this as it's finally starting to block us (unless we want a fork of SEAL, but eww). HEXL works, but compiling the generated dotnet projects outside the source directory appears not to. I'll investigate a bit more, but dotnet has changed so much since I was at Microsoft 4 years ago. If I'm not able to figure out how to build dotnet outside of the source tree, would it be okay to add a flag to disable dotnet project generation and just generate them where they are today?

@rickwebiii
Copy link
Author

@WeiDaiWD Ok, got dotnet building outside the sourcetree. The CSharp tests passed and I'm able to run the examples on a Linux AWS VM.

Also, the examples+tests were previously broken on Linux as we weren't copying libsealc.so, only the ones with a .number after them. That's now fixed.

@WeiDaiWD
Copy link
Contributor

Sorry for dealing with so late. These changes are amazing. I like this pure-source-tree experience. The only issue with this PR is that dotnet/SEALNet.sln no longer works. I've created a fix for that, which will soon be released in 3.7.3. Thank you so much for your contribution.

@WeiDaiWD WeiDaiWD merged commit e121860 into microsoft:contrib Mar 15, 2022
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.

2 participants