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 TestHarness python integration scripts to dev package #818

Merged
merged 20 commits into from
Mar 17, 2023

Conversation

oschwaldp-oci
Copy link
Contributor

@oschwaldp-oci oschwaldp-oci commented Mar 14, 2023

Provide the utilities of the TestHarness framework in the dev-install package of leap.

Now, when building leap, as part of the make dev-install installation, the TestHarness, as well as the contracts used in testing in libraries/testing/contracts and unittests/contracts will be made available for use outside of a leap source build tree.

TestHarness Updates:

To make this possible, changes were needed in TestHarness to allow similar directory structures in both source build and install directory structures:

  • core_symbol.py was moved into the TestHarness module and can now be altered by setting the CORE_SYMBOL_NAME environment variable. Otherwise it defaults to SYS.
  • logging.json was left in place in the source tree for useful reference, however, in the build tree it has been placed in the TestHarness directory and renamed to logging-template.json to better express its use in the framework.
  • Cluster.py now makes use of relative path references to find the launcher.py and contracts used for setting up test infrastructure.
  • testUtils.py now makes use of the programs from the bin/ directory instead of programs/* to make referencing in the install directory structure possible. The TestLogs and /etc/eosio/ logging directories are both now shifted to reference the current working directory for when making use of the utility outside of the build tree.

Example Installing Use Case

To build, make sure you are in the root of the leap repo, then run the following commands:
mkdir -p build
cd build

Using make dev-install

Select where to install the leap dev package using CMAKE_INSTALL_PREFIX:
cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_INSTALL_PREFIX=~/leap_install_dir ..

Build and install leap dev:
make dev-install

Using make package

Select where to install the leap dev package using CMAKE_INSTALL_PREFIX:
cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_INSTALL_PREFIX=~/leap_install_dir ..

Build leap dev package:
make package

Note When doing the make package step, it is not a true error if you see:
Error creating directory "/<leap_install_dir /lib/python3/dist-packages".
CMake Error: failed to create symbolic link '/<leap_install_dir >/lib/python3/dist-packages/TestHarness': no such file or directory
CMake Error: failed to create symbolic link '/<leap_install_dir >/share/leap_testing/bin': no such file or directory

It is caused by a necessary step for make install to work on cmake versions < 3.21 but can flag as an error in the package step since those directories and symbolic links are created in the postinit script during the package install instead. Couldn't find a way to differentiate between make package or make install in the cmake/makefile.

Then install the dev and base packages:

For testing, this was done in a container, but the steps are:

apt-get update
apt-get install -y ./leap-dev_4.0.0-dev-ubuntu20.04_amd64.deb
apt-get install -y ./leap_4.0.0-dev-ubuntu20.04_amd64.deb

Validating install

This will install leap to the ~/leap_install_dir. Of note within the chosen install location for the dev package:

  • ~/leap_install_dir/share/leap_testing/ -> where the dev testing components are installed
  • /leap_testing/libraries/testing/contracts/ -> contracts used during testing
  • /leap_testing/unittests/contracts/ -> contracts used during testing
  • /leap_testing/tests/TestHarness/ -> testing framework and utilities
  • /leap_testing/bin -> symbolic link pointing to ~/leap_install_dir/bin to allow relative path referencing of programs by the TestHarness
  • ~/leap_install_dir/bin -> installed versions of cleos, eosio-blocklog, keosd, leap-util, nodeos, trace_api_util
  • ~/leap_install_dir/lib/python3/dist-packages/TestHarness -> symbolic link pointing to installed TestHarness used for python path for installed module discovery.

Using Installed dev package

  • Update the PYTHONPATH environment variable to allow python to find the installed TestHarness module. eg.:
    • export PYTHONPATH=~/leap_install_dir/lib/python3/dist-packages
  • Optional: Update CORE_SYMBOL_NAME:
    • export CORE_SYMBOL_NAME=EOS
  • Import necessary components from TestHarness into python script:
    • from TestHarness import Cluster, TestHelper, Utils, WalletMgr, CORE_SYMBOL
    • from TestHarness.TestHelper import AppArgs

Resolves: #707

CMakeLists.txt Outdated
PATTERN "CMakeFiles" EXCLUDE
PATTERN "*.cmake" EXCLUDE
PATTERN "Makefile" EXCLUDE)
install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/unittests/contracts DESTINATION ${CMAKE_INSTALL_FULL_DATAROOTDIR}/leap_testing/unittests COMPONENT base
Copy link
Member

Choose a reason for hiding this comment

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

should this have been COMPONENT dev here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Good catch. Thanks.

Addressed: 6f16347

CMakeLists.txt Outdated

install(CODE "execute_process( COMMAND ${CMAKE_COMMAND} -E make_directory ${CMAKE_INSTALL_FULL_LIBDIR}/python3/dist-packages)" COMPONENT dev)
install(CODE "execute_process( COMMAND ${CMAKE_COMMAND} -E create_symlink ../../../share/leap_testing/tests/TestHarness ${CMAKE_INSTALL_FULL_LIBDIR}/python3/dist-packages/TestHarness)" COMPONENT dev)
install(CODE "execute_process( COMMAND ${CMAKE_COMMAND} -E create_symlink ../../bin ${CMAKE_INSTALL_FULL_DATAROOTDIR}/leap_testing/bin)" COMPONENT dev)
Copy link
Member

Choose a reason for hiding this comment

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

How do these work for the leap-dev.deb package? I downloaded the leap-dev.deb from the CI run, and when I

ar x leap-dev_4.0.0-dev-ubuntu20.04_amd64.deb
tar tlf data.tar.gz  | less

I don't see these symlinks anywhere. And I don't see any thing else magical from cmake in the package either. Admittedly I didn't try actually installing...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The symbolic links are now created in the install step making use of the postinit script that has been added. Had to jump through some hoops since there are issues with create_symlink in cmake versions < 3.22, so there are 2 paths now based on cmake versions. Added some notes in the PR description.

Copy link
Contributor

@ScottBailey ScottBailey Mar 16, 2023

Choose a reason for hiding this comment

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

Try it with the verbose flag, like this:

ar x leap-dev_4.0.0-dev-ubuntu20.04_amd64.deb data.tar.gz
tar tlvf data.tar.gz | rg "\->"

If you haven't installed ripgrep yet, what are you waiting for? Its half the typing of grep! that alone is a win!

Here's my result from a local build:

lrwxrwxrwx root/root         0 2023-03-16 09:00 ./opt/lib/python3/dist-packages/TestHarness -> ../../../share/leap_testing/tests/TestHarness
lrwxrwxrwx root/root         0 2023-03-16 09:00 ./opt/share/leap_testing/bin -> ../../bin

@heifner heifner added documentation Improvements or additions to documentation OCI Work exclusive to OCI team labels Mar 15, 2023
Existing steps were failing to work with package and old versions of cmake.
Don't add cwd to the front of the already absolute path. Fixes error where unix socket path  was too long.
CMakeLists.txt Outdated
PATTERN "CMakeFiles" EXCLUDE)
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/tests/launcher.py DESTINATION ${CMAKE_INSTALL_FULL_DATAROOTDIR}/leap_testing/tests COMPONENT dev)

if(${CMAKE_VERSION} GREATER 3.22)
Copy link
Member

Choose a reason for hiding this comment

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

Probably should use VERSION_GREATER here (or maybe even VERSION_GREATER_EQUAL to make it a little clearer); also no need to expand the variable.

Can you explain a little more what the problem is for pre vs post 3.22 (3.23?)? For example, you only set the CPACK_DEBIAN_PACKAGE_CONTROL_EXTRA in the pre-3.22/3 path; is it really unneeded in the post-3.22/3 path? Is it really not possible to just use the pre-3.22/3 path on all versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just double checking, you're suggesting:

if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.22)

Yeah I can get on board with that, it is clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain a little more what the problem is for pre vs post 3.22 (3.23?)? For example, you only set the CPACK_DEBIAN_PACKAGE_CONTROL_EXTRA in the pre-3.22/3 path; is it really unneeded in the post-3.22/3 path? Is it really not possible to just use the pre-3.22/3 path on all versions?

Because of the use of install(CODE and the false error reporting that comes out of that piece during a make package, was hoping to leave the >3.22 path in place with the hopes of remove the <3.22 path when we get to a point where we aren't supporting older versions of cmake where the create_symlink function has yet to be fixed. And also alleviate that reporting for anyone building/installing on newer cmake toolchains.

Copy link
Member

Choose a reason for hiding this comment

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

Yes but if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.22) may be different then if(${CMAKE_VERSION} GREATER 3.22) because the original might not have been true for 3.22.0 since 3.22.0 might not have been greater than 3.22. So double check which version you really want here maybe it's 3.22 or 3.23

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it is actually due to cmake (fixed in 3.21) not allowing symbolic links to a directory to be installed via install(FILES

Cmake 3.21 fixed this (https://gitlab.kitware.com/cmake/cmake/-/commit/d71a7cc19d6e03f89581efdaa8d63db216184d40)
Allow symbolic links to a directory to be installed via install(FILES ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed: be154da

CMakeLists.txt Outdated
install(CODE "execute_process( COMMAND ${CMAKE_COMMAND} -E create_symlink ../../../share/leap_testing/tests/TestHarness ${CMAKE_INSTALL_FULL_LIBDIR}/python3/dist-packages/TestHarness)" COMPONENT dev)
install(CODE "execute_process( COMMAND ${CMAKE_COMMAND} -E create_symlink ../../bin ${CMAKE_INSTALL_FULL_DATAROOTDIR}/leap_testing/bin)" COMPONENT dev)

set(CPACK_DEBIAN_PACKAGE_CONTROL_EXTRA ${CMAKE_BINARY_DIR}/scripts/postinst COMPONENT dev)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this set() should have a COMPONENT dev.. that doesn't seem right. It seems like instead you'd want set(CPACK_DEBIAN_DEV_PACKAGE_CONTROL_EXTRA ${CMAKE_BINARY_DIR}/scripts/postinst). I also would suggest considering placing this inside of package.cmake along with all the other package related stuff to keep all package related stuff together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed: be154da


if [ ! -L ${CMAKE_INSTALL_FULL_LIBDIR}/python3/dist-packages/TestHarness ]; then
mkdir -p ${CMAKE_INSTALL_FULL_LIBDIR}/python3/dist-packages
ln -s ../../../share/leap_testing/tests/TestHarness ${CMAKE_INSTALL_FULL_LIBDIR}/python3/dist-packages/TestHarness
Copy link
Member

Choose a reason for hiding this comment

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

Instead of ${CMAKE_INSTALL_FULL_LIBDIR}/python3/dist-packages, would ${Python_SITELIB} be more appropriate since the system python installation directory doesn't depend on the installation path of leap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spoonincode

Per our discussion:

So, from what I can tell there are a couple caveats here:

It's populated for free after executing FindPackage(Python), but only since CMake 3.12.
So that leaves me having to do something in the cmake file like:
execute_process(COMMAND python3 -c "from distutils import sysconfig;print(sysconfig.get_python_lib(plat_specific=False,standard_lib=False))" OUTPUT_VARIABLE Python_SITELIB)

But am not sure if finding Python_SITELIB from cmake is a good idea.
Which means we could push that into the preinst and prerm scripts. But then we lose the ability to use relative paths in the symbolic link we're creating. Which then causes some additional weirdness for the make install path vs. the make package path.

From our discussion, sounds like the thought is to:

Leave it as is for now. Then when dropping support for ubuntu18 and bumping cmake we can tend to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I concur, finding Python_SITELIB on the host is probably not optimal.

But we could potentially use /usr/lib/python3/dist-packages. It would work for Debian Bookworm and Ubuntu 18 through 23.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue created to investigate this in the future: #839

CMakeLists.txt Outdated
@@ -257,6 +257,40 @@ configure_file(programs/cleos/LICENSE.CLI11 licen

install(DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/licenses/leap" DESTINATION "${CMAKE_INSTALL_FULL_DATAROOTDIR}/licenses/" COMPONENT base)

install(DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/libraries/testing/contracts DESTINATION ${CMAKE_INSTALL_FULL_DATAROOTDIR}/leap_testing/libraries/testing COMPONENT dev
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, for any place you have a COMPONENT dev it should also be EXCLUDE_FROM_ALL.

The problem is, we want make install to only install the files from the base component (or put another way, we want make install to ignore anything we classify as a dev install), but there is no way afaik to control the behavior of make install since it's a built in target that can't be overridden. So we have to resort to peeling away all these from the "install all" target.

The reason we go through this trouble is that historically, before the COMPONENT dev EXCLUDE_FROM_ALL behavior, a make install could install header files that were in a path with higher precedence than our own source tree. As you can imagine this led to users (and devs!) getting in to really wonky problems when the, for example, fc headers in /usr/local/include would be used instead of the fc headers in the current source tree. There is a little more about discussion on this in eosnetworkfoundation/mandel#277

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed: be154da

@spoonincode
Copy link
Member

We should probably add python3-numpy to the dev package DEPENDS list

set(CPACK_DEBIAN_DEV_PACKAGE_DEPENDS "libboost-all-dev, libssl-dev, libgmp-dev")

Is there any other external applications the python scripts depend on? I recall we got rid of curl in #442 that's the only one that comes to mind for me.

scripts/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@ScottBailey ScottBailey left a comment

Choose a reason for hiding this comment

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

Please address Matt's issues including adding the debian control script prerm.

scripts/postinst Outdated Show resolved Hide resolved
Adding additional commentary on cmake version based processing paths.

Added prerm script to cleanup symbolic links

Added EXCLUDE_FROM_ALL where necessary.

Move setting scripts to the package.cmake location suggested.
@oschwaldp-oci
Copy link
Contributor Author

We should probably add python3-numpy to the dev package DEPENDS list

set(CPACK_DEBIAN_DEV_PACKAGE_DEPENDS "libboost-all-dev, libssl-dev, libgmp-dev")

Is there any other external applications the python scripts depend on? I recall we got rid of curl in #442 that's the only one that comes to mind for me.

Did a quick scan of the dependencies and didn't find any outside python3-numpy like you already mentioned.
Addressed: be154da

@oschwaldp-oci
Copy link
Contributor Author

Please address Matt's issues including adding the debian control script prerm.

Mostly addressed here: be154da

CMakeLists.txt Outdated Show resolved Hide resolved
scripts/prerm Outdated Show resolved Hide resolved
Simplify prerm script.
Copy link
Contributor

@ScottBailey ScottBailey left a comment

Choose a reason for hiding this comment

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

lgtm

package.cmake Outdated
@@ -70,6 +70,8 @@ if(DPKG_QUERY AND OS_RELEASE MATCHES "\n?ID=\"?ubuntu" AND LLVM_CMAKE_DIR)
string(APPEND CPACK_DEBIAN_DEV_PACKAGE_DEPENDS ", ${LLVM_PKG_FIND_OUTPUT}")
endif()
endif()
set(CPACK_DEBIAN_DEV_PACKAGE_CONTROL_EXTRA ${CMAKE_BINARY_DIR}/scripts/postinst)
set(CPACK_DEBIAN_DEV_PACKAGE_CONTROL_EXTRA ${CMAKE_BINARY_DIR}/scripts/prerm)
Copy link
Member

Choose a reason for hiding this comment

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

This causes only prerm to be bundled. Probably should be a cmake list I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying:
set(CPACK_DEBIAN_DEV_PACKAGE_CONTROL_EXTRA ${CMAKE_BINARY_DIR}/scripts/postinst;${CMAKE_BINARY_DIR}/scripts/prerm)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested and that worked:
Addressed: 8c8ad50

Copy link
Member

@spoonincode spoonincode left a comment

Choose a reason for hiding this comment

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

looks good, thanks

package.cmake Outdated Show resolved Hide resolved
Comment on lines +3 to +4
rm -f ${CMAKE_INSTALL_FULL_LIBDIR}/python3/dist-packages/TestHarness
rm -f ${CMAKE_INSTALL_FULL_DATAROOTDIR}/leap_testing/bin
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I should have caught this before.

Suggested change
rm -f ${CMAKE_INSTALL_FULL_LIBDIR}/python3/dist-packages/TestHarness
rm -f ${CMAKE_INSTALL_FULL_DATAROOTDIR}/leap_testing/bin
rm -rf ${CMAKE_INSTALL_FULL_LIBDIR}/python3/dist-packages
rm -rf ${CMAKE_INSTALL_FULL_DATAROOTDIR}/leap_testing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm nervous about this change. What if it is installed into a common location where python3/dist-packages/ could have non-leap owner resources we don't want to just destroy. I almost think I need to check at each level and only remove if the dir is empty.

Copy link
Member

Choose a reason for hiding this comment

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

yeah I agree, I don't think we should touch anything other then the symlinks we created. I would even consider not mkdiring these: leap_testing is created & removed by way of the package anyways. dist-packages isn't really ours to own; and our dependency on python3-numpy (ergo python3) guarantees it exists before us anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point about dist packages. You could do a combination 'rmdir -p' for the directors and rm -r for the symlinks, and that would protect us, I think.

Copy link
Member

Choose a reason for hiding this comment

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

if you want to make it as simple as possible, I'd suggest removing the mkdirs since afaict they're guaranteed to be there by the time the postinst script runs. Then you're only responsible for removing the symlinks in prerm (unchanged from how it is now).

That allows the postinst & prerm to be 100% the opposite of each other without worrying about removing directories and whatnot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the solution I implemented and tested: c46400c

Best effort attempt to remove the directories we may have created during the install.

Can ignore failed directory deletion as they may be installing to location that has more than leap in it, for instance the ~/lib/python3/dist-packages, however if it is empty it is a safe assumption that it is in the location the install created it and can be removed as the inverse uninstall operation.

Copy link
Contributor

@ScottBailey ScottBailey left a comment

Choose a reason for hiding this comment

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

Almost there! A couple quick changes to add. :-)

oschwaldp-oci and others added 3 commits March 16, 2023 18:11
Co-authored-by: Scott B <Scott@TheBaileyFamily.org>
…ng the install.

Can ignore failed directory deletion as they may be installing to location that has more than leap in it, for instance the ~/lib/python3/dist-packages, however if it is empty it is a safe assumption that it is in the location the install created it and can be removed as the inverse uninstall operation.
scripts/prerm Outdated
rm -f ${CMAKE_INSTALL_FULL_DATAROOTDIR}/leap_testing/bin
rmdir --ignore-fail-on-non-empty ${CMAKE_INSTALL_FULL_LIBDIR}/python3/dist-packages
rmdir --ignore-fail-on-non-empty ${CMAKE_INSTALL_FULL_LIBDIR}/python3
rmdir --ignore-fail-on-non-empty ${CMAKE_INSTALL_FULL_DATAROOTDIR}/leap_testing
Copy link
Member

Choose a reason for hiding this comment

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

when/how could this be empty so that it would succeed? Seems impossible: leap_testing is part of the package. Feels like we're over complicating this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You have a point, for leap_testing at least since that is part of the package. Can probably remove the mkdir from the postinst script for leap_testing and the rmdir for it in prerm.

However, I think we need to keep the python3/dist-packages as it may be created from scratch in the postinst and isn't in the package. Thus we should also attempt to clean it up.

Copy link
Member

Choose a reason for hiding this comment

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

Since the package depends on python3-numpy it's impossible for /usr/lib/python3/dist-packages/ to not exist at the time of postinst

Now, of course, we're not linking to /usr/lib/python3/dist-packages/ but rather ${CMAKE_INSTALL_FULL_LIBDIR}/python3/dist-packages; that's kind of the disappointment with not being able to use ${Python_SITELIB} as it would eliminate that academic difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplified per discussion and tested, still works: e7e66a5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no strict guarantee that someone is installing to /usr/lib/python3/dist-packages so this solution was formulated to take that into account and still allow it to work. It also takes into account that with the requirement of running with cmake 3.8, as we discussed, the use of ${Python_SITELIB} wouldn't currently work as we were intending.

Copy link
Member

Choose a reason for hiding this comment

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

There is an important distinction though: while someone may be installing many different versions to various CMAKE_INSTALL_PREFIXs via make dev-install, they can, by rule, only install a single leap-dev.deb. So it seems plausible to consider the installed package's TestHarness as the "system provided" TestHarness no matter where it is installed.

That actually makes me wonder if instead the postinst should be something like

ln -s ${CMAKE_INSTALL_FULL_DATAROOTDIR}/leap_testing/tests/TestHarness ${Python_SITELIB}/TestHarness

as this will make the installed package's TestHarness the system provided TestHarness no matter what CMAKE_INSTALL_PREFIX was used at build time (which should be /usr per-spec, but a user might not have configured it that way)

Anyways, we can debate this further post 4.0, where we actually have ${Python_SITELIB}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about this a lot, even before you posted this comment. I tend to agree with you here. It would be good to treat it that way. As you said, let's revisit this. I'll create an issue to track this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spoonincode - Issue created to investigate this in the future: #839

leap_testing dir is already part of package so don't need to mkdir or rmdir as it will be cleaned up appropriately.

However, continue to clean up dir potentially created directly by postinst
Copy link
Contributor

@ScottBailey ScottBailey left a comment

Choose a reason for hiding this comment

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

lgtm

install(FILES ${CMAKE_CURRENT_BINARY_DIR}/tests/launcher.py DESTINATION ${CMAKE_INSTALL_FULL_DATAROOTDIR}/leap_testing/tests COMPONENT dev EXCLUDE_FROM_ALL)

if(CMAKE_VERSION VERSION_GREATER_EQUAL 3.21)
# Cmake versions < 3.21 did not support installing symbolic links to a directory via install(FILES ...)
Copy link
Member

Choose a reason for hiding this comment

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

thanks for putting the very clear explanation around this

Base automatically changed from eosio-launcher-python-port to main March 17, 2023 16:04
@oschwaldp-oci oschwaldp-oci merged commit 77dfd0c into main Mar 17, 2023
@oschwaldp-oci oschwaldp-oci deleted the dev-install-add-testharness branch March 17, 2023 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation OCI Work exclusive to OCI team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add TestHarness python integration scripts to dev package
4 participants