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.

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

Jump to bottom

Replace apr (Abandoned) #79

Closed
wants to merge 24 commits into from
Closed

Replace apr (Abandoned) #79

wants to merge 24 commits into from

Conversation

dkeeney
Copy link

@dkeeney dkeeney commented Sep 26, 2018

Creating a pull request

@dkeeney
Copy link
Author

dkeeney commented Sep 26, 2018

I will repeat my comments...now that I am on the right repro. I messed up and the PR went to the numenta repository. I had a lot of confused people over there :(

Status
I have this all working under C++17 on ubuntu and cpp-8. They compile and all unit tests run. There are a few boost routines still around that could be replaced but I will get those later.
BUT, None of the CI's seem to like anything. It appears that we don't have the right compiler in the CI.

I implemented the os::Path and os::Directory modules using #if __has_include ( < filesystem > ). See os::Directory.hpp line 45. It will attempt to #include< filesystem > but if that is not found it includes <experimental/filesystem> and if that is not found, fall back to <boost/filesystem.hpp>. The boost solution of course will require boost system and filesystem libraries which we do not have in the build.

I am developing with Ubuntu using cpp-8. I know that the latest cpp-8 does have < filesystem > but it still requires library -lstdc++fs. I got it to link by putting the -lstdc++fs after the objects in the link.

cpp-7 has <experimental/filesystem> but links with the experimental library. From what I have read, the latest versions of most compilers will now support < filesystem > but some of them still require the -lstdc++fs library. MinGW does not seem to support filesystem at all so boost is the only alternative. Not sure what support CLang has although Apple's version of it does not support filesystem yet.

Visual Studio 17 (latest) does have < filesystem > and it builds and runs just fine but of course this version of my code already has SWIG removed because I don't think it will build SWIG. I did verify that if I forced it to use boost::filesystem rather that std::filesystem it will also build and run successfully under windows.

The point is that I am having second thoughts about requiring C++17. Every Compiler/platform combination seems to have a different setup for supporting filesystem. Maybe we should go with boost::filesystem and C++11 until the compilers all comply fully with the C++17 standard.

@breznak
Copy link
Member

breznak commented Sep 30, 2018

Thanks David,
don't worry about the PR in numenta :)

BUT, None of the CI's seem to like anything. It appears that we don't have the right compiler in the CI.

Yes, the CIs are running antique versions of compilers (compared to c++17), see {.travis,circleci,appveyor}.yaml files. I think we can (and should) bump to newer versions (might need to install it to the CI)

using #if __has_include ( < filesystem > ). See os::Directory.hpp line 45. It will attempt to #include< filesystem > but if that is not found it includes <experimental/filesystem> and if that is not found, fall back to <boost/filesystem.hpp>

I'd propose to use just <boost/fs> for now. When we do boost removal in other PR, we'll solve the c++17 stuff.

The boost solution of course will require boost system and filesystem libraries which we do not have in the build.

It's a bit pity we can use just the includes of boost. The c++17 will resolve that again.

I know that the latest cpp-8 does have < filesystem > but it still requires library -lstdc++fs. I got it to link by putting the -lstdc++fs after the objects in the link.

About the mess with link flags for , I don't think it should be a big obstacle, we have platform specific flangs in CMake anyways, we'd just set it up once..

The point is that I am having second thoughts about requiring C++17. Every Compiler/platform combination seems to have a different setup for supporting filesystem. Maybe we should go with boost::filesystem and C++11 until the compilers all comply fully with the C++17 standard.

As above, I'd think before this PR/repo matures, newer version of compilers will be again around. But I do see your concerns. Let's go the boost:: way now, removing APR and friends, and we'll try to remove boost in a final push.

  • my concern is need to actually build boost (it's a huge monster, would take time in CI), we cannot use only the includes for the required functionality, I assume?

Filesystem is supported in g++8 (in Ubuntu 18.04 LTS, I think), and clang 7, MSVC17
https://en.cppreference.com/w/cpp/compiler_support

breznak
breznak requested changes Sep 30, 2018
View reviewed changes
Copy link
Member

@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.

Additional comments to the PR itself

  • You've somehow changed so many files (whitespace), please try to limit that and exclude it from the PR. It's fairly difficult to review now. Do you know how did that happen?
  • if we need to build the Boost, let's make that a PR alone - again, huge changes. Just switching from includes-only to build boost-fs ... After that, this PR should be much more feature-focused.

CommonCompilerConfig.cmake Outdated

##### Set parameters

set(INTERNAL_CPP_STANDARD "-std=c++11")
Copy link
Member

Choose a reason for hiding this comment

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

I think this is just more confusing than stating it directly, unless we have a good usecase.

Dockerfile Outdated
FROM ubuntu:14.04

RUN apt-get update && \
boost install -y \
Copy link
Member

Choose a reason for hiding this comment

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

we don't use Docker...

@dkeeney
Copy link
Author

dkeeney commented Sep 30, 2018 via email

@dkeeney
Copy link
Author

dkeeney commented Sep 30, 2018 via email

@dkeeney
Copy link
Author

dkeeney commented Oct 10, 2018

I have successfully added boost install to the package so that we can have boost::filesystem under std++11 instead of std::filesystem under std++17. This worked quite well for Linux but... for MinGW the boost install is "major pain and suffering". The hardest problem is that the Boost installation generates .o objects (ELF format) and all other objects are .obj (COFF/PE format). The resulting static library cannot be a mixture of object formats. The resulting executables do not run. I got around this problem with a hack to convert the boost ELF objects to COFF/PE objects before being added to the combined library.

Now that I got past that, there is a problem building the bindings that is failing. Some file is not being found...have to research this.

We are not really using MinGW but rather a fork of it, mingwpy to generate Python compatable SWIG bindings for use on Windows. As of 10/14/2017 the mingwpy project has been abandoned. I think that when we remove SWIG all of this will no longer be necessary. We should scrap MinGW support and implement Visual Studio 2017.

@breznak
Copy link
Member

breznak commented Oct 10, 2018

I have successfully added boost install to the package so that we can have boost::filesystem under std++11 instead of std::filesystem under std++17

Great news! 👍

for MinGW the boost install is "major pain and suffering".

sorry to hear that, the fake Windows env is a pain.

We should scrap MinGW support and implement Visual Studio 2017.

I agree, it costs is much time. I'd propose to make a MSVC build- just the c++ stuff, no swig/pytests and run that. In future we'll transition away from swig and will make full python build on Windows available again. What do you think?

@dkeeney
Copy link
Author

dkeeney commented Oct 10, 2018 via email

@dkeeney
Copy link
Author

dkeeney commented Oct 10, 2018

The OSx build is working. The Linux build (travis) puzzles me because I cannot reproduce the error locally. Here it works great. It does not seem to be the compiler version either. I will keep looking.

breznak
breznak reviewed Oct 11, 2018
View reviewed changes
Copy link
Member

@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.

reviewed the last changes, overall nice cleanup 👍

.travis.yml Outdated
packages:
# - g++-4.9
- g++-7
Copy link
Member

Choose a reason for hiding this comment

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

nice bump, maybe we should specify this in the Readme as well.

external/Boost.cmake Outdated

######################################################################
# MinGW Notes: MinGW needs special handling.
# 1) The default Library filename is something like libboost_filesystem-mgw49-mt-x64-1_68.a
Copy link
Member

Choose a reason for hiding this comment

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

minor: let's make a note we'd like to transition from MingGW to clean MSVC

src/CMakeLists.txt Outdated
@@ -29,7 +29,7 @@ message(STATUS "CMAKE_CURRENT_SOURCE_DIR = ${CMAKE_CURRENT_SOURCE_DIR}")
message(STATUS "EP_BASE = ${EP_BASE}")

set_property(GLOBAL PROPERTY USE_FOLDERS ON)
set(CMAKE_VERBOSE_MAKEFILE OFF) # toggle for cmake debug
set(CMAKE_VERBOSE_MAKEFILE ON) # toggle for cmake debug
Copy link
Member

Choose a reason for hiding this comment

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

TODO: before finishing the PR, toggle back to OFF

src/CombineUnixArchives.cmake Outdated
# i.e., Windows with MINGW toolchain
set(globbing_ext "obj")
# i.e., MSYS & MINGW toolchain; could be .o or .obj
set(globbing "${working_dir}/*.o*")
Copy link
Member

Choose a reason for hiding this comment

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

nit: would this cover all cases? (but keep the doc which format uses which OS)

src/CombineUnixArchives.cmake Outdated
@@ -104,6 +107,23 @@ function(COMBINE_UNIX_ARCHIVES
# Use relative paths to work around too-long command failure when building
# on Windows in AppVeyor
file(RELATIVE_PATH new_obj_file_path ${scratch_dir} ${new_obj_file_path})

####### remove this section if we no longer support MINGW
get_filename_component(ext ${new_obj_file_path} EXT)
Copy link
Member

Choose a reason for hiding this comment

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

is MINGW actually working? if not, and likely won't?, let's remove it right away

@dkeeney
Copy link
Author

dkeeney commented Oct 11, 2018

I am going to set this project aside for a while. I am getting obsessed with it and need a break. We are making a little trip to Mexico and will be gone for a week. Maybe when I get back I will look at this and it will be obvious what the problem is.

@dkeeney
Copy link
Author

dkeeney commented Oct 11, 2018 •
edited

Changes in "remove_APR" PR to date:
Comparing remove_APR branch against current master

  1. Removed APR, apr-iconv, apr-util and yaml from external/common/share
    yaml was not being used. Keeping Yaml-cpp which is being used.
    external/CMakeLists.txt
    external/Apr1Lib.cmake (deleted)
    external/AprUtil1.Lib.cmake (deleted)
    external/commong/share/apr, apr-iconv, apr-util,yaml (deleted)

  2. Removed boost include files and added a full boost install
    so we can get the system and filesystem libraries as well as the include files.
    external/common/share/boost/*.hpp (removed)
    Boost.cmake (new)
    external/CMakeLists.txt

  3. implemented a superBuild with all of external dependancies
    as one external project and nupic core library as the second which depends on the first.
    The dependancies are found using find_package() rather than having the references passed.
    CMakeLists.txt
    src/CMakeLists.txt
    external/CMakeLists.txt
    Swig.cmake
    Boost.cmake
    YamlCppLib.cmake
    CommonCompilerConfig.cmake (CPP standard is passed in)
    CombineUnitxArchives.cmake (trying to get MinGW to work)
    NupicLibraryUtils.cmake

  4. Replaced APR file handling with boost::filesystem or std::filesystem
    Directory.cpp/hpp
    Path.cpp
    OS.cpp
    PathTest.cpp
    DirectoryTest.cpp

5 Replaced APR functions with C++11 std:: functions
NuPIC.cpp
Timer.cpp/hpp
TimerTest.cpp
Env.cpp/hpp
FStream.cpp
OS.cpp/hpp
OSUnix.cpp
OSWin.cpp
UnitTestMain.cpp

  1. Replaced boost::circular_buffer with std::deque
    Link.cpp/hpp
    RegionImplFactory.cpp

  2. Replaced boost::shared_ptr with std::shared_ptr
    When I started I thought this would be C++17 so I was
    removing all references to boost. When we fell back to
    C++11 these still worked so I retained those changes.
    Not all boost references were removed.
    RegionImplFactory.cpp
    TestNode.cpp
    UniformLinkPolicy.cpp/hpp
    YamlUtils.cpp
    Buffer.cpp/hpp
    Value.cpp/hpp -- also the string value did not need shared_ptr.
    PyRegion.cpp
    YAMLUtilsTest.cpp
    ArrayTest.cpp
    ValueTest.cpp

  3. Removed boost::concept_check
    Math.hpp

  4. Replaced boost::unordered_set with std::unordered_set
    SparseMatrix.hpp

  5. Changes in Stlio.hpp and Utils.hpp ended up being only formatting.
    Spend a lot of time messing with Functors and ended up without substantive changes.

  6. Removed the NTA_ types; NTA_Int32 to Int32, etc.
    Buffer.cpp/hpp

  7. Replaced boost::type_index for std::type_index
    NumpyVector.hpp

  8. Added a trim() function (needed for Path.cpp) and replaced some APR functions.
    StringUtils.cpp/hpp

  9. added f and u to numeric constants
    These were to remove compiler warnings.
    ConnectionsTest.cpp
    SDRClassifierTest.cpp
    SegmentTest.cpp
    SpatialPoolerTest.cpp
    TemporalMemoryTest.cpp
    YAMLUtilsTest.cpp
    SegmentMatrixAdapterTest.cpp
    TopologyTest.cpp
    ExceptionTest.cpp
    RandomTest.cpp

  10. replaced ASSERT_EQ(false, bool value) with ASSERT_FALSE(bool value)
    The MinGC compiler did not like it.
    SDRClassifierTest.cpp
    ScalarTest.cpp

  11. Added more testing for Array. (actually something that got left out of previous PR)
    and some fixes to exception handling.
    ArrayTest.cpp

17 Formatting only
Cell.cpp
Cells4.cpp/hpp

@dkeeney
Copy link
Author

dkeeney commented Oct 12, 2018

Could not help taking one last crack at it before I leave for a week. But still breaks at the same place.

@dkeeney
Copy link
Author

dkeeney commented Oct 20, 2018

I am back from my little vacation.
I have Linux and OSx working for this PR.

MinGW I am giving up on for now. MinGW needs the Boost build to use the special version of the gcc compiler tailored to work with Python 2.7 to build the Python SWIG bindings for windows. I may be able to eventually get that to work but I don't think it is worth the effort. What I am hoping is that with the PyBind11 interface this special compiler will not be a requirement. I can then create a build that will work for both MinGW and Visual Studio 2017.

This PR is currently running with C++11 and Boost for filesystem. Later we can turn on the ability to use C++17 and std::filesystem for those build environments that can handle it.

So, @breznak, is there any cleanup on this PR I still need to do before proceeding with a new PR? I was thinking about extracting a subset of Boost that just builds the filesystem and system modules. I think this is the only thing left that I need Boost for. This would avoid the download step. I would take the sources for just those modules and add them to the external folder rather than install all of the Boost includes and the filesystem module. Another thing I can do is add some checks to see if an acceptable version of boost is already installed someplace on a system before performing a build of the filesystem module. Or, should I just leave it as is?

I am thinking that the next step would be to isolate the Python related interface code into a separate ExternalProject section in such a way that the nupic.cpp library does not contain any Python interface code. This is still using SWIG. Then once that is working, another PR could construct a parallel ExternalProject to use PyBind11 as the Python interface. Or, should I just discard the SWIG bindings and go for the PyBind11 interface all in one go?

@dkeeney
[av skip] cleanned up minor things. Tried to remove as many calls to …
98e0d50 
…boost as possible. There are still several besides filesystem.
@dkeeney
Copy link
Author

dkeeney commented Oct 24, 2018

The diffs for the 'replace_APR' branch are showing a lot of files with whitespace changes. Many of these are files that I never even opened in an editor. It could be something to do with line endings changing. At one point I was using GIT on windows and now I am using Ubuntu. That could have something to do with it but I don't really understand what happened.
It is fairly easy to see the changes I made in this PR by comparing the PR's branch against the Master using 'Beyond Compare'.

So, now that the Linux and OSx builds are successful, How do we get this PR completed?

@dkeeney
Copy link
Author

dkeeney commented Nov 5, 2018

I am going to start over with this PR.
I need to break it up into many smaller parts and avoid the whitespace changes (mostly line endings changing from /LF to /LF/CR).

So, I will start by adding Boost filesystem and system modules with one PR, Then do a PR for each file to remove the APR routines. Then as the last PR, remove the APR libraries.

@dkeeney dkeeney mentioned this pull request Nov 5, 2018
Closed
1 task
@dkeeney dkeeney changed the title Replace apr Replace apr (Abandoned) Nov 5, 2018
@dkeeney dkeeney mentioned this pull request Nov 13, 2018
Merged
@breznak
Copy link
Member

breznak commented Nov 13, 2018

@dkeeney I managed to fix the whitespace, merged master with boost changes, and made small fixes

Do you want to revive this PR, where you've put a lot of work, or keep it as review and do the separate PRs? I think it'd be possible to do a few rounds of reviews of this PR and get it merged.

Current problems:

  • linker Boost system/fs fails
  • I'd prefer to get rid of the super-build concept and keep CMake files as in master now

@dkeeney
Copy link
Author

dkeeney commented Nov 13, 2018

The CMake files will eventually have to be re-organized, (probably with the super build) when we separate the Python interface and possibly testing, in order to get the dependencies right.

But lets break this up into separate PR's. What I am proposing is that I create a separate PR for each .cpp file (and its .hpp). I will be pulling lines from a copy of this PR to do this so it should not take long to do. By doing this the changes will get a proper review.

@breznak
Copy link
Member

breznak commented Nov 13, 2018

The CMake files will eventually have to be re-organized, (probably with the super build) when we separate the Python interface and possibly testing, in order to get the dependencies right.

I thouught this could be in the hierarchical cmake, like #91 suggests ?

/ 
/src/nupic/
/src/tests
/bindings/{lang}/

would all get its (sub)cmake

But let's leave that when necessary

But lets break this up into separate PR's. What I am proposing is that I create a separate PR for each .cpp file (and its .hpp). I will be pulling lines from a copy of this PR to do this so it should not take long to do. By doing this the changes will get a proper review.

Maybe not exactly file-by-file, but make it common-sense small (like your points in #81 (comment) , that's a good level for PR)
Ok, I'll be looking forward to PRs 👋

@dkeeney dkeeney mentioned this pull request Nov 13, 2018
Merged
@breznak
Copy link
Member

breznak commented Nov 15, 2018

@dkeeney ready to close this PR? There is a conflict in the merge now, likely not worth solving, but I quickly checked the diff and seems all the functionality is covered?

@dkeeney
Copy link
Author

dkeeney commented Nov 15, 2018 via email

@breznak
Copy link
Member

breznak commented Nov 15, 2018

All merged already in separate PRs.

@breznak breznak closed this Nov 15, 2018
@breznak breznak deleted the replace_APR branch November 15, 2018 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@breznak breznak breznak requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@dkeeney @breznak