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

[3.1] fix libff in 3.1 tester cmakes #502

Merged
merged 5 commits into from
Jun 24, 2022

Conversation

oschwaldp-oci
Copy link
Contributor

@oschwaldp-oci oschwaldp-oci commented Jun 22, 2022

Fix issues seen with libff when trying to use mandel with mandel.cdt.

Update CMakeLists.txt to create a parallel mandel-config.cmake for use until mandel goes through renaming.

Update path to libff in fc libraries when using build from src.

Add install directions for current mandel use with mandel.cdt (or others).

Resolves: #426
Resolves: #420

Supersedes: #421

Relates to mandel.cdt issue/PR:
eosnetworkfoundation/mandel.cdt#25
eosnetworkfoundation/mandel.cdt#26

…el.cdt.

Update CMakeLists.txt to create a parallel mandel-config.cmake for use until mandel goes through renaming.

Update path to libff in fc libraries when using build from src.

Add install directions for current mandel use with mandel.cdt (or others).
@oschwaldp-oci oschwaldp-oci changed the title GH-426 -- fix libff in 3.1 tester cmakes fix libff in 3.1 tester cmakes Jun 22, 2022
README.md Outdated
cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_INSTALL_PREFIX=/usr/local ..
make -j $(nproc)
make install
cmake --install . --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.

make -j $(nproc) dev-install will combine all 3 of these lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated: b955a8b

README.md Outdated
git submodule update --init --recursive
mkdir build
cd build
cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_INSTALL_PREFIX=/usr/local ..
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 want to encourage users to install dev components in to /usr/local because it causes conflicts. Will need to use something else (idk what we've been indecisive; maybe $HOME/mandeldev 🤷 ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: 2d05d35

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.

EosioTesterBuild.cmake is used when users do not want to install the dev files to use them for native contract user testing. EosioTester.cmake is used when the dev files are installed.

Does EosioTester.cmake need to be changed too? This PR is only changing EosioTesterBuild.cmake.

(this is part of the reason I'm not really sure how to feel about encouraging users to install the dev files at all -- it's not strictly needed)

@oschwaldp-oci
Copy link
Contributor Author

oschwaldp-oci commented Jun 22, 2022

EosioTesterBuild.cmake is used when users do not want to install the dev files to use them for native contract user testing. EosioTester.cmake is used when the dev files are installed.

Understood.

Does EosioTester.cmake need to be changed too? This PR is only changing EosioTesterBuild.cmake.

EosioTesterBuild.cmake needed to be updated due to changes made to fc and where libff was moved.
EosioTester.cmake still makes use of @CMAKE_INSTALL_PREFIX@ to find libff. When doing the dev install instructions, libff is still installed in @CMAKE_INSTALL_PREFIX@/lib -- thus no changes were needed there.

(this is part of the reason I'm not really sure how to feel about encouraging users to install the dev files at all -- it's not strictly needed)

Understood. However, the option to install still currently exists with little, if any, documentation. Was trying to smooth the path to making use of it for a user. We may still want to make changes to the install methodology or remove it if no longer wanted/supported/needed.

@spoonincode
Copy link
Member

Okay, we definitely do not want to encourage /usr/local though. It's a disaster. I'd even say there needs to be something in the README warning users to place it elsewhere since /usr/local is the default

@spoonincode
Copy link
Member

I even toyed with the idea of disabling the dev component when the install prefix is /usr or /usr/local, but didn't go through with it. Maybe that's another option we could entertain.

@oschwaldp-oci
Copy link
Contributor Author

Okay, we definitely do not want to encourage /usr/local though. It's a disaster. I'd even say there needs to be something in the README warning users to place it elsewhere since /usr/local is the default

That sounds like a good idea. Is there a specific location you'd like to use? Do you like your previous suggestion of $HOME/mandeldev enough to move forward with that? I can make something generic and then connect the dots between mandel and mandel.cdt README instructions.

@larryk85
Copy link
Contributor

Okay, we definitely do not want to encourage /usr/local though. It's a disaster. I'd even say there needs to be something in the README warning users to place it elsewhere since /usr/local is the default

That sounds like a good idea. Is there a specific location you'd like to use? Do you like your previous suggestion of $HOME/mandeldev enough to move forward with that? I can make something generic and then connect the dots between mandel and mandel.cdt README instructions.

I would recommend making this one generic so $HOME/mandeldev is fine with me.

For the most part in the future devs should have the dev package available so this instruction would be for outliers anyway.

@heifner heifner added the OCI OCI working this issue... label Jun 23, 2022
@spoonincode
Copy link
Member

Yeah that's the thing, you don't need to install it to use it with cdt, so do we really want to encourage installing it in the first place? (With a marque entry in the README no less)

The existing mandel-contracts documentation already is written as not performing an install and just using the build dir. Do we really feel an alternative approach to our own suggestion in that repo needs to be prominent and encouraged here?

@oschwaldp-oci
Copy link
Contributor Author

Yeah that's the thing, you don't need to install it to use it with cdt, so do we really want to encourage installing it in the first place? (With a marque entry in the README no less)

The existing mandel-contracts documentation already is written as not performing an install and just using the build dir. Do we really feel an alternative approach to our own suggestion in that repo needs to be prominent and encouraged here?

Fair point. How would you prefer to proceed? Simply delete the section on installation or is there a less marquee location to document this as a reference for someone specifically looking?

@larryk85
Copy link
Contributor

Yeah that's the thing, you don't need to install it to use it with cdt, so do we really want to encourage installing it in the first place? (With a marque entry in the README no less)
The existing mandel-contracts documentation already is written as not performing an install and just using the build dir. Do we really feel an alternative approach to our own suggestion in that repo needs to be prominent and encouraged here?

Fair point. How would you prefer to proceed? Simply delete the section on installation or is there a less marquee location to document this as a reference for someone specifically looking?

I would recommend just leaving the install step out of the README for now. We really should be moving to using the dev packages for this use case anyway. So, the documentation for mandel-contracts can stay as it is now, but move to listing the dev package and installation of that as the way to build it's contracts.

@spoonincode
Copy link
Member

Yeah my hunch is to shy away from even mentioning it in the README.

However, it is a behavior change compared to pre 3.1, so I could see something in the release notes long the lines of

For users utilizing native contract unit testing, make install no longer installs the required header and libraries. It's recommended to utilize the headers and libraries directly from mandel's build directory, or to use the mandel-dev package. Installation of these headers and libraries can still be performed with a make dev-install if absolutely needed.

Mandel dev install is no longer the recommendation.

For users utilizing native contract unit testing, make install no longer installs the required header and libraries. It's recommended to utilize the headers and libraries directly from mandel's build directory, or to use the mandel-dev package. Installation of these headers and libraries can still be performed with a make dev-install if absolutely needed.
@oschwaldp-oci
Copy link
Contributor Author

Yeah my hunch is to shy away from even mentioning it in the README.

However, it is a behavior change compared to pre 3.1, so I could see something in the release notes long the lines of

For users utilizing native contract unit testing, make install no longer installs the required header and libraries. It's recommended to utilize the headers and libraries directly from mandel's build directory, or to use the mandel-dev package. Installation of these headers and libraries can still be performed with a make dev-install if absolutely needed.

Sounds like a plan. I will remove the install directions from the README.

@oschwaldp-oci oschwaldp-oci merged commit 8684d55 into release/3.1.x Jun 24, 2022
@oschwaldp-oci oschwaldp-oci changed the title fix libff in 3.1 tester cmakes fix libff in 3.1 tester cmakes [3.1] Jun 24, 2022
@arhag arhag changed the title fix libff in 3.1 tester cmakes [3.1] [3.1] fix libff in 3.1 tester cmakes Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCI OCI working this issue...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants