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

MSVC support, CI github Actions windows-2019 #2098

Merged
merged 89 commits into from
Mar 10, 2022

Conversation

quentin
Copy link
Member

@quentin quentin commented Oct 11, 2021

This is a work-in-progress to support the build of Souffle on native Windows with Visual Studio.

Current achievement:

  • build on Github Actions with windows-latest.
  • tests are disabled, they all fail in ctest currently, I did not investigate.

Fixes
Main fixes include:

  • must use signed integer loop variable in OMP parallel for.
  • must explicit many integer conversions.
  • must not use dynamically sized array, use unique pointers instead.
  • must replace gnu builtins with equivalent msvc builtins.

Other changes:

  • getopt / getopt_long no standard on Windows, wrote a replacement function to avoid introducing third-party licensed code.
  • some compilation options.

@quentin
Copy link
Member Author

quentin commented Oct 11, 2021

The new GH action needs a dependency that is blocked in this repo, probably for security reasons.

Error : .github#L1
lukka/run-vcpkg@v7 is not allowed to be used in souffle-lang/souffle. Actions in this workflow must be: within a repository owned by souffle-lang, verified in the GitHub Marketplace or match the following: softprops/action-gh-release@*, styfle/cancel-workflow-action@*, webfactory/ssh-agent@*.

@b-scholz would you add an exception rule for lukka/run-vcpkg@v7 ? It is required to access the Microsoft c/c++ package manager that resolves some dependencies for the Windows build.

Comment on lines 68 to 76
// The separator in the PATH variable
#ifdef _MSC_VER
const char PATHdelimiter = ';';
const char pathSeparator = '\\';
#else
const char PATHdelimiter = ':';
const char pathSeparator = '/';
#endif
Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, would switch to std::filesystem

@b-scholz
Copy link
Member

The new GH action needs a dependency that is blocked in this repo, probably for security reasons.

Error : .github#L1
lukka/run-vcpkg@v7 is not allowed to be used in souffle-lang/souffle. Actions in this workflow must be: within a repository owned by souffle-lang, verified in the GitHub Marketplace or match the following: softprops/action-gh-release@*, styfle/cancel-workflow-action@*, webfactory/ssh-agent@*.

@b-scholz would you add an exception rule for lukka/run-vcpkg@v7 ? It is required to access the Microsoft c/c++ package manager that resolves some dependencies for the Windows build.

Can you give us instructions how to do this?

@quentin
Copy link
Member Author

quentin commented Oct 12, 2021

The setting should be available from the project's "Settings > Actions > Allow select actions" as discussed here for instance

@b-scholz please let me know when it's fixed so I can relaunch the action. Thanks.

The new GH action needs a dependency that is blocked in this repo, probably for security reasons.

Error : .github#L1
lukka/run-vcpkg@v7 is not allowed to be used in souffle-lang/souffle. Actions in this workflow must be: within a repository owned by souffle-lang, verified in the GitHub Marketplace or match the following: softprops/action-gh-release@*, styfle/cancel-workflow-action@*, webfactory/ssh-agent@*.

@b-scholz would you add an exception rule for lukka/run-vcpkg@v7 ? It is required to access the Microsoft c/c++ package manager that resolves some dependencies for the Windows build.

Can you give us instructions how to do this?

@codecov
Copy link

codecov bot commented Oct 12, 2021

Codecov Report

Merging #2098 (4dff141) into master (6c731e5) will decrease coverage by 0.07%.
The diff coverage is 77.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2098      +/-   ##
==========================================
- Coverage   75.58%   75.51%   -0.08%     
==========================================
  Files         452      452              
  Lines       27266    27477     +211     
==========================================
+ Hits        20609    20748     +139     
- Misses       6657     6729      +72     
Impacted Files Coverage Δ
src/ast/ExecutionOrder.h 100.00% <ø> (ø)
src/ast/Program.h 100.00% <ø> (ø)
src/ast/analysis/typesystem/TypeSystem.h 84.33% <ø> (ø)
src/ast/transform/ComponentInstantiation.cpp 91.98% <ø> (ø)
src/ast/transform/MinimiseProgram.h 100.00% <ø> (ø)
src/ast/transform/ResolveAliases.cpp 98.93% <ø> (ø)
src/ast/utility/SipsMetric.h 18.18% <ø> (ø)
src/ast/utility/Utils.h 100.00% <ø> (ø)
src/ast/utility/Visitor.h 57.33% <0.00%> (ø)
src/ast2ram/provenance/ClauseTranslator.h 100.00% <ø> (ø)
... and 105 more

@b-scholz
Copy link
Member

The flyweight test-case fails. Should we re-run the tests?

@b-scholz
Copy link
Member

Also there are conflicts now.

@quentin
Copy link
Member Author

quentin commented Oct 20, 2021

@b-scholz I fixed the conflicts, the issue with the flyweight test-case is unrelated to this change and was fixed by #2105.

Note that the windows CI still fails to start because lukka/run-vcpkg@v7 is not allowed to be used in souffle-lang/souffle [...].

@b-scholz
Copy link
Member

I added the exception. Thanks for the instructions. I hope it works now.

@b-scholz
Copy link
Member

I see that the tests are passing now (without the exception) - is this correct?

@quentin
Copy link
Member Author

quentin commented Oct 21, 2021

@b-scholz Thanks, I this time the VS-CI-Tests is starting correctly.

I re-enabled the tests for Windows. I re-wrote some (not all of them yet) of the sh scripts to ruby for portability on Windows and started adapting the cmake test files accordingly.

I chose ruby as a first draft, but I can move to another language as long as it is available both on Windows, Linux and Mac. This rewriting strategy must be discussed/reviewed, I am just making this as a suggestion.

@b-scholz
Copy link
Member

Tom suggested Python. I am not familiar with Windows. Python might be more mainstream and might be available for Windows as well.

@quentin quentin force-pushed the try-msvc branch 2 times, most recently from 0cb6fd1 to e3fcb60 Compare January 13, 2022 17:12
@quentin
Copy link
Member Author

quentin commented Jan 14, 2022

Hi @b-scholz,
I finally got something that pass compilation and tests. I rewrote all the testing scripts and souffle-compile in Python.

Current state on Linux / MacOS

  • Ported common feature of compile-souffle to compile-souffle.py, but swig and a few other options are not ported yet.
  • All tests pass.

Current state on Windows with Visual Studio

  • profiling is entirely disabled. I think it's relying on non-portable code and I did not attempted to port it.
  • Independent Github CI file: VS-CI-Tests.yaml, configured for windows-latest runner, with OpenMP support.
  • All tests pass. Except the profiling tests that are disabled. And a few tests that I disabled in the CMake files for various reasons with an explanation comment.

TODO before merging

  • Finish souffle-compile.py to support rare use cases like swig.
  • Cleanup old souffle-compile generation, remove scripts that I wrote initially.

@@ -504,51 +504,41 @@ void SemanticCheckerImpl::checkClause(const Clause& clause) {
}

void SemanticCheckerImpl::checkComplexRule(const std::set<const Clause*>& multiRule) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Basically std::set does not guarantee that Clauses will be ordered in a deterministic way because it is ordered by pointer's value.
By chance it seems that on Linux and MacOS the clauses are allocated memory location with the same order as the lexical order of clauses in the rule.
Unfortunately on Visual Studio, the clauses memory locations are not deterministically ordered.

// complex rule only once.
// TODO (b-scholz): for negation / disjunction this is not quite
// right; we would need more semantic information here.
for (auto literal : (*multiRule.begin())->getBodyLiterals()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The first clause returned by begin() is not deterministic when compiled with Visual Studio.

Copy link
Member

Choose a reason for hiding this comment

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

Oh dear - perhaps it is related to pointers rather than semantic info for ordering in STL containers. We had such issues in the past.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. My successive changes in this functions are mostly useless and could be entirely roll-back.

If we really want begin() to always return the "first" clause (lexically), then we should provide a dedicated Comparator to the multiRule set. And that comparator would somehow dig into the pointed clauses, explore the body source-locations and compute the "less" relation.

But my feeling is that there is a more general problem and just relying on the "first" clause is not sufficient to correctly check the whole rule.

@@ -227,4 +227,8 @@ positive_test(pragma1)
souffle_run_test_helper(TEST_NAME pragma2 FUNCTORS CATEGORY semantic)
positive_test(rel_redundant)
positive_test(type_as4)
if (FALSE)
# semantics checker of complex rule does not catch this
# "variable only occurs once" issue.
positive_test(complex_rule)
Copy link
Member Author

Choose a reason for hiding this comment

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

This test exhibit a more general issue with SemanticCheckerImpl::checkComplexRule, as commented by b-scholtz in the function:

    // Count the variable occurrence for the body of a
    // complex rule only once.
    // TODO (b-scholz): for negation / disjunction this is not quite
    // right; we would need more semantic information here.

@b-scholz
Copy link
Member

What is still missing for completion?

Comment on lines +100 to +111
if (NOT (MSVC AND (CMAKE_BUILD_TYPE MATCHES Debug)))
# When compiled in Debug with Visual Studio with the default stack size of 1MB,
# the interpreter fails with a stack overflow in Engine::execute().
# The cause is the number of lambda functions that are allocated as locals on the stack.
# The stack frame of Engine::execute is close to 30KB.
# The 1MB stack will overflow at depth ~34 of Engine::execute().
positive_test(magic_nqueens)
endif ()
Copy link
Member Author

Choose a reason for hiding this comment

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

This should probably be addressed as an issue.
Engine::execute must be split into smaller pieces to decrease the size required by the stack-frame.

@quentin
Copy link
Member Author

quentin commented Jan 14, 2022

What is still missing for completion?

  • souffle-compile.py to support rare use cases like swig -s. It's a bit of an effort to test swig, because it's not part of the default tests, so I need a bit of time to setup the correct environment with swig. Help would be welcome.
  • souffle-compile.py to support -t/-w (but I'm not sure those two options have an actual value) ?
  • Cleanup old souffle-compile generation, remove scripts that I wrote initially (easy).

@quentin
Copy link
Member Author

quentin commented Jan 24, 2022

I made sure the SWIG tests pass on Linux.
Removed the old souffle-compile and the initial Ruby scripts.

@quentin
Copy link
Member Author

quentin commented Jan 25, 2022

Hi @b-scholz, the port to Windows is now in a good shape.

Should the 70+ commits of this change be added in the master tree as-is? I cannot guarantee (actually I am pretty sure) that the intermediate steps don't build successfully.

@XiaowenHu96 The new dependency on Python 3.x may need to be taken care of in some of the distribution/packaging scripts?

@XiaowenHu96
Copy link
Collaborator

Hi Quentin!

If I understand it correctly, souffle-compile is replaced with a python script? Then yes, we need to add Python3 as a runtime dependency. I think you need to modify two files:

  1. top-level CMakeList.txt, search for CPACK_RPM_PACKAGE_REQUIRES and CPACK_DEBIAN_PACKAGE_DEPENDS.
  2. .github/images/arch-linux/PKGBUILD.in, edit the depends list.

@quentin quentin changed the title WIP: CI build on Github Actions windows-latest MSVC support, CI github Actions windows-2019 Mar 5, 2022
@b-scholz
Copy link
Member

b-scholz commented Mar 7, 2022

Could you check that the SWIG interface is still working? We don't have SWIG as part of our CI.

@quentin
Copy link
Member Author

quentin commented Mar 7, 2022

Hi @b-scholz, the SWIG interface tests are working on Linux, for both Python and Java.
Why not enabling SWIG in the Linux CI, as well as examples ?

Do you want me to create a clean pull-request with all changes squashed into a single commit ?

@b-scholz
Copy link
Member

b-scholz commented Mar 7, 2022

I think it is not necessary to squash everything into a single commit. That is alright.

Could you enable SWIG testing? I think it would be good to test this - otherwise, it will stop working soon.

@quentin
Copy link
Member Author

quentin commented Mar 7, 2022

I suggest we enable SWIG testing in the CI with a separate change, it will probably have some impacts on dependencies (Python, Java).

Copy link
Member

@b-scholz b-scholz left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@b-scholz b-scholz merged commit 4226e49 into souffle-lang:master Mar 10, 2022
@quentin quentin deleted the try-msvc branch March 15, 2022 16:27
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.

3 participants