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

[ENH] Dev-ops, N+1 offset, temporary directory #5

Draft
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

anibalsolon
Copy link

@anibalsolon anibalsolon commented Nov 20, 2022

This PR:

  • changes cmake setup to use conan as its library manager
  • downloads and points unit tests to the data dir
  • changes the logic to use N+1 offset
  • implements the temp dir logic from python
  • documents on how to run things
  • improves example
  • improves unit testing based on the python implementation
  • implements benchmark run/stats
  • add CI using Github Actions

@anibalsolon anibalsolon marked this pull request as draft November 20, 2022 22:29
@anibalsolon anibalsolon requested a review from frheault November 23, 2022 14:51
Copy link
Contributor

@frheault frheault left a comment

Choose a reason for hiding this comment

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

I think the changes look great!

The next item on the list is about .
Maybe after that item, I can try to run the test on my new computer and see if it works?

@arokem
Copy link
Contributor

arokem commented Jan 9, 2023

On my mac, I am currently seeing:

(base) ➜  build git:(enh/devops) cmake .. && make
-- The C compiler identification is AppleClang 11.0.3.11030032
-- The CXX compiler identification is AppleClang 11.0.3.11030032
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /Library/Developer/CommandLineTools/usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /Library/Developer/CommandLineTools/usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Conan: Adjusting output directories
-- Conan: Using cmake global configuration
-- Conan: Adjusting default RPATHs Conan policies
-- Conan: Adjusting language standard
-- Current conanbuildinfo.cmake directory: /Users/arokem/source/trx-cpp/build
-- Found GTest: /Users/arokem/.conan/data/gtest/cci.20210126/_/_/package/aafb7f47234990db9077ed87e3f6c6c9c0e84a95/lib/libgtest.a  
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/arokem/source/trx-cpp/build
[ 25%] Building CXX object CMakeFiles/trx.dir/src/trx.cpp.o
In file included from /Users/arokem/source/trx-cpp/src/trx.cpp:1:
In file included from /Users/arokem/source/trx-cpp/include/trx/trx.h:297:
/Users/arokem/source/trx-cpp/src/trx.tpp:1089:39: error: use of undeclared
      identifier 'canonicalize_file_name'
        std::string directory = (std::string)canonicalize_file_name(path...
                                             ^
1 error generated.
make[2]: *** [CMakeFiles/trx.dir/src/trx.cpp.o] Error 1
make[1]: *** [CMakeFiles/trx.dir/all] Error 2
make: *** [all] Error 2

@frheault
Copy link
Contributor

Great! It all worked!

Just so you know, here are small things I had to do:

After creating the profile, CONAN warn me of 'WARNING: GCC OLD ABI COMPATIBILITY' and told me I likely needed to run this command.
conan profile update settings.compiler.libcxx=libstdc++11 .conan
image

Then since I used CONAN before when I ran conan install --build=missing --settings=build_type=Debug --profile ./.conan .. I had to add the --update parameters.

Then CMAKE warned me that GTEST was missing,
image
so I googled it and I installed this:
sudo apt-get install libgtest-dev

And then the build and test worked!

PS: I see the file in ./examples/load_trx.cpp should work but and I see it in the CMAKE, but where is it being built?

@anibalsolon
Copy link
Author

@frheault good that it is working now- yes, this command is included in the README

The gtest should have been installed via conan, not sure why it didnt find

@frheault
Copy link
Contributor

frheault commented Feb 6, 2023

@anibalsolon Was I doing something wrong or was the file in ./examples/load_trx.cpp not build?

@frheault
Copy link
Contributor

@anibalsolon Is there something I can run or test before of meeting this afternoon?

@anibalsolon
Copy link
Author

@frheault not really- we may need to discuss some design decisions about the cpp implementation today

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