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

CI: build in Docker with MUSL libc for static library #736

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

breznak
Copy link
Member

@breznak breznak commented Oct 30, 2019

For #707 #19

avoid setting "static" for glibc, as glibc does not really support
that. Use Musl if we want static libstdc++
now possible that we use libyaml instead of yaml-cpp
if exists on host, as it likely contains incompatible
build files
- "fixes" (by providing custom) deterministic results for MUSL libc
runtime
- pass NTA_LIBC_MUSL from command-line when needed (in Docker which runs
on musl/Alpine)
- properly build as static library (cannot be on GLIBC)
Copy link
Member Author

@breznak breznak left a comment

Choose a reason for hiding this comment

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

Please have a look, this is now working, but some points could be probably done better (see below).

TODO:

  • build PyPI for amd64 also on Docker (has working static lib build)

@@ -157,12 +157,17 @@ jobs:
build-debug:
name: Build and test in Debug mode
#currently cannot run on Linux & Debug due to a bug in YAML parser: issue #218
runs-on: macOS-latest
runs-on: ubuntu-18.04
Copy link
Member Author

Choose a reason for hiding this comment

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

switching to ubuntu for we can use gcc-8 (c++17) there

CMakeLists.txt Outdated Show resolved Hide resolved
set(stdlib_cxx ${stdlib_cxx} -stdlib=libc++)
endif()

# TODO: investigate if we should use static or shared stdlib and gcc lib.
Copy link
Member Author

Choose a reason for hiding this comment

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

-static never worked properly with gcc, so we'll use musl for the binary releases.

@@ -34,7 +34,7 @@
if(EXISTS "${REPOSITORY_DIR}/build/ThirdParty/share/googletest.tar.gz")
set(URL "${REPOSITORY_DIR}/build/ThirdParty/share/googletest.tar.gz")
else()
set(URL https://github.com/abseil/googletest/archive/release-1.8.1.tar.gz)
set(URL https://github.com/abseil/googletest/archive/release-1.10.0.tar.gz)
Copy link
Member Author

Choose a reason for hiding this comment

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

bump gtest

external/gtest.cmake Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
@@ -200,7 +195,11 @@ EPOCHS = 2; // make test faster in Debug

SDR goldSP({COLS});
const SDR_sparse_t deterministicSP{
#ifdef NTA_LIBC_MUSL
62, 72, 73, 82, 85, 102, 263, 277, 287, 303, 306, 308, 309, 322, 337, 339, 340, 352, 370, 493, 1094, 1095, 1114, 1115, 1120, 1463, 1512, 1518, 1647, 1651, 1691, 1694, 1729, 1745, 1746, 1760, 1770, 1774, 1775, 1781, 1797, 1798, 1803, 1804, 1805, 1812, 1827, 1828, 1831, 1832, 1858, 1859, 1860, 1861, 1862, 1875, 1878, 1880, 1881, 1898, 1918, 1923, 1929, 1931, 1936, 1950, 1953, 1956, 1958, 1961, 1964, 1965, 1967, 1971, 1973, 1975, 1976, 1979, 1980, 1981, 1982, 1984, 1985, 1986, 1988, 1991, 1994, 1996, 1997, 1998, 1999, 2002, 2006, 2008, 2011, 2012, 2013, 2017, 2019, 2022, 2027, 2030
Copy link
Member Author

Choose a reason for hiding this comment

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

added custom deterministic results for MUSL. I don't know if we can achieve same on glibc & musl.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #707

@breznak breznak self-assigned this Oct 30, 2019
@breznak breznak requested a review from dkeeney October 30, 2019 20:08
dkeeney
dkeeney previously approved these changes Oct 30, 2019
Copy link

@dkeeney dkeeney left a comment

Choose a reason for hiding this comment

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

This is ok as is but see my comments.
What I do is run a build and see where the lib ends up. then go change the link path accordingly.

CMakeLists.txt Outdated Show resolved Hide resolved
external/gtest.cmake Show resolved Hide resolved
external/gtest.cmake Show resolved Hide resolved
as the CI does not have a TTY
use option() rather than set()
the CI does "docker run" without -it, so we need to run
python tests directly, not using multi-line commands.
@breznak breznak requested a review from dkeeney October 31, 2019 06:28
@breznak breznak changed the title Avoid trying static glibc WIP CI: build in Docker with MUSL libc for static library Oct 31, 2019
@breznak breznak closed this Oct 31, 2019
@breznak breznak reopened this Oct 31, 2019
@breznak breznak closed this Oct 31, 2019
@breznak breznak reopened this Oct 31, 2019
Copy link

@dkeeney dkeeney left a comment

Choose a reason for hiding this comment

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

COMMON_COMPILER_DEFS seems to be empty in src/CMakeLists,

It should not be empty. If it is then we are not getting any of our settings. COMMON_COMPILER_DEFS and its string version COMMON_COMPILER_DEFINITIONS_STR should be inherited from the top CMakeList.txt. The string version of it is displayed in a message statement there.

target_compile_definitions(${src_dyn_executable_hotgym} PRIVATE ${COMMON_COMPILER_DEFINITIONS})
if(${NTA_LIBC_MUSL})
target_compile_definitions(${src_dyn_executable_hotgym} PRIVATE ${COMMON_COMPILER_DEFINITIONS} -DNTA_LIBC_MUSL=${NTA_LIBC_MUSL})
else()
Copy link

Choose a reason for hiding this comment

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

${COMMON_COMPILER_DEFINITIONS} -DNTA_LIBC_MUSL=${NTA_LIBC_MUSL})

Should not need to do that if it is already in COMMON_COMPILER_DEFINITIONS.

@breznak
Copy link
Member Author

breznak commented Oct 31, 2019

https://github.com/htm-community/htm.core/pull/736/checks?check_run_id=283078173#step:5:1176

I'm unable to reproduce locally (but using the same Docker), @dkeeney when you have time, would you please test for me on another system? You'll need docker, and then follow the Dockerfile instructions at the top of the file (or see the CI section), please?

@dkeeney
Copy link

dkeeney commented Oct 31, 2019

I don't know anything about Docker so this is a good time to find out. I can put my other PR on hold for now.

Now, what you are trying to do is use the static C++ runtime library and you are doing it with this MUSL thing. I see three builds that fail...are they one for each platform?

@dkeeney
Copy link

dkeeney commented Oct 31, 2019

I ran into a MAJOR snag. Docker Desktop for windows requires Hyper-V to work. VirtualBox does not work at all with Hyper-V turned on. I use VirtualBox as my Ubuntu installation. I cannot install Docker inside another virtualization either. There does not appear to be any way around this short of buying another computer.

@dkeeney
Copy link

dkeeney commented Oct 31, 2019

There is an older Docker Toolbox that does run Docker under VirtualBox. I could use that but it might not be the environment that you want me to try.

@breznak
Copy link
Member Author

breznak commented Nov 1, 2019

I don't know anything about Docker so this is a good time to find out.

thanks a lot for willing to try that for me, David!

are trying to do is use the static C++ runtime library and you are doing it with this MUSL thing

MUSL is an alternative stdlibc++ for linux, most commonly used is glibc (only which we've been using so far), MSVC has its version, etc. GLIBC is known for its static libraries (when you link stdlibc in your code) are not truly independent, transferable. Unlike musl, where this should work ok. The intention is to use such statically linked lib as a binary for all linux distributions on PyPI

I see three builds that fail...are they one for each platform?

it's the UX here messed up, it's the same platform - x86_64 on Alpine linux (with musl C) ran inside docker. I've restarted the PR and the old results just keep showing up.

Docker Desktop for windows requires Hyper-V to work. VirtualBox does not work at all with Hyper-V turned on. I use VirtualBox as my Ubuntu installation

can you enable HyperV in BIOS, or is it a HW limitation? If you could, you'd just test the docker from Windows, and then disable HyperV again to be able to run VBox. I was not aware of such complication on Win, sorry. On linux (running directly on HW) I run both VBox and Docker containers.

https://docs.docker.com/docker-for-windows/install/#what-to-know-before-you-install

@dkeeney
Copy link

dkeeney commented Nov 1, 2019

can you enable HyperV in BIOS

Ok, I think I understand what you are looking for. Yes it is possible to toggle HyperV in the BIOS.
I am leaving this morning for a few days in San Jose with our grandson. Will be back Monday afternoon. I will give this a try when I return.

@breznak
Copy link
Member Author

breznak commented Nov 1, 2019

Enjoy your weekend! 🍂
PS: I'm able to replicate the bug locally, crash in Classifier, leads to someting with NTA_CHECK macros and way how we handle exceptions. Preparing a PR that aims to streamline to process of our exceptions. (removing LogItem and the likes)

@dkeeney
Copy link

dkeeney commented Nov 1, 2019

(removing LogItem and the likes)

Oh, very good. That is still some old original code. Could use a cleanup.

@breznak breznak mentioned this pull request Nov 1, 2019
@dkeeney
Copy link

dkeeney commented Nov 9, 2019

Do you still need me to try docker on my machine? I am reluctant to risk messing up my Ubuntu OS which is under virtualbox but if you still need me to do this I will give it a try.

@breznak
Copy link
Member Author

breznak commented Nov 9, 2019

Do you still need me to try docker on my machine?

so far I'm good, or I can replicate the err seen in CI. Thank you very much for offering the help!

@breznak breznak added platform and removed ready labels Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants