Skip to content

Conversation

@jlheflin
Copy link
Contributor

Is this pull request associated with an issue(s)?
No

Description
Unable to get the binaries that are built to link the hello_world program correctly.

@jlheflin jlheflin marked this pull request as draft December 29, 2024 04:11
@ryanmrichard
Copy link
Member

The changes I pushed make your include paths more canonical; however, I think this PR should have worked without them. Regardless, I can verify that in its current state this PR builds locally for me (and I suspect CI will pass too, but I'm leaving this comment before it finishes).

@jlheflin
Copy link
Contributor Author

@ryanmrichard I realized I was a bit too vague, when running the tests via ctest in the build directory, the test executable itself shows this error:

/home/jacob/Projects/PluginPlay/build/test_unit_pluginplay: symbol lookup error: /home/jacob/Projects/PluginPlay/build/test_unit_pluginplay: undefined symbol: _Z11hello_worldB5cxx11v

I've tried a few things to make sure that it would be able to be found but I'll try your changes as well. The project itself will build, but the tests do not run correctly.

@jlheflin
Copy link
Contributor Author

Additionally, the library itself has a reference to the function, when using nm I get the following: 0000000000086cb0 T hello_worldabi:cxx11

@ryanmrichard
Copy link
Member

I (as well as CI) am also running ctest in the build directory and don't see this problem. If that's your only undefined symbol my guess is you're linking to old version of PluginPlay (not the one with your new function).

Based on that assumption, if I were you, I'd create a completely new workspace say $HOME/new_workspace. I'd then clone PluginPlay into that workspace say $HOME/new_workspace/pluginplay. I'd then create a toolchain file say $HOME/new_workspace/toolchain.cmake. I'd then make sure all the search and install paths in the toolchain point to paths in $HOME/new_workspace, i.e., don't reuse anything from previous builds. I also recommend NOT installing PluginPlay, just build the tests and run them. As a sanity check I'd do this all with the master branch first. Once that builds and runs, I'd pull the GitHub mermaid-graph branch, delete the build directory, and reconfigure, build, and test.

@jlheflin
Copy link
Contributor Author

Yeah you nailed it, my LD_LIBRARY_PATH was pulling from my NWChemEx install with its own libpluginplay.so. Once I removed this reference the tests completed.

*/

#pragma once
#include "module_manager/module_manager_class.hpp"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#include "module_manager/module_manager_class.hpp"
#include <pluginplay/module_manager/module_manager_class.hpp>

Long story short, if a header file lives in the include/ directory you should include it with angle brackets (and start the path with the directory which follows include/ in the path, which in this case is pluginplay)

@jlheflin
Copy link
Contributor Author

Here is the resulting output for the integration test of GhostFragment for the Mermaid Graph:

flowchart LR
	AAA[All nmers]
	AAA-->|Monomer Maker| AAB[No Submodule associated with Key]
Loading
flowchart LR
	AAC[Atomic Capping]
Loading
flowchart LR
	AAD[Bond-Based Fragmenter]
Loading
flowchart LR
	AAE[Broken Bonds]
Loading
flowchart LR
	AAF[Cluster Partition]
Loading
flowchart LR
	AAG[Covalent Radius]
Loading
flowchart LR
	AAH[DCLC Capping]
	AAH-->|Connectivity| AAI[Covalent Radius]
Loading
flowchart LR
	AAJ[Energy Method]
Loading
flowchart LR
	AAK[Fragment Based Method]
	AAK-->|Energy method| AAL[Energy Method]
	AAK-->|Subsystem former| AAM[FragmentedChemicalSystem Driver]
	AAM-->|Fragmenter| AAN[Fragment Driver]
	AAN-->|Atomic connectivity| AAO[Covalent Radius]
	AAN-->|Cap broken bonds| AAP[Weighted Distance]
	AAN-->|Find broken bonds| AAQ[Broken Bonds]
	AAN-->|Fragment builder| AAR[Bond-Based Fragmenter]
	AAN-->|Intersection finder| AAS[Intersections]
	AAN-->|Molecular graph| AAT[Nuclear Graph]
	AAT-->|Connectivity| AAU[Covalent Radius]
	AAT-->|Nodes| AAV[Heavy Atom Partition]
	AAV-->|Connectivity| AAW[Covalent Radius]
	AAN-->|N-mer builder| AAX[All nmers]
	AAX-->|Monomer Maker| AAY[No Submodule associated with Key]
	AAK-->|Weighter| AAZ[GMBE Weights]
Loading
flowchart LR
	ABA[Fragment Driver]
	ABA-->|Atomic connectivity| ABB[Covalent Radius]
	ABA-->|Cap broken bonds| ABC[Weighted Distance]
	ABA-->|Find broken bonds| ABD[Broken Bonds]
	ABA-->|Fragment builder| ABE[Bond-Based Fragmenter]
	ABA-->|Intersection finder| ABF[Intersections]
	ABA-->|Molecular graph| ABG[Nuclear Graph]
	ABG-->|Connectivity| ABH[Covalent Radius]
	ABG-->|Nodes| ABI[Heavy Atom Partition]
	ABI-->|Connectivity| ABJ[Covalent Radius]
	ABA-->|N-mer builder| ABK[All nmers]
	ABK-->|Monomer Maker| ABL[No Submodule associated with Key]
Loading
flowchart LR
	ABM[FragmentedChemicalSystem Driver]
	ABM-->|Fragmenter| ABN[Fragment Driver]
	ABN-->|Atomic connectivity| ABO[Covalent Radius]
	ABN-->|Cap broken bonds| ABP[Weighted Distance]
	ABN-->|Find broken bonds| ABQ[Broken Bonds]
	ABN-->|Fragment builder| ABR[Bond-Based Fragmenter]
	ABN-->|Intersection finder| ABS[Intersections]
	ABN-->|Molecular graph| ABT[Nuclear Graph]
	ABT-->|Connectivity| ABU[Covalent Radius]
	ABT-->|Nodes| ABV[Heavy Atom Partition]
	ABV-->|Connectivity| ABW[Covalent Radius]
	ABN-->|N-mer builder| ABX[All nmers]
	ABX-->|Monomer Maker| ABY[No Submodule associated with Key]
Loading
flowchart LR
	ABZ[GMBE Weights]
Loading
flowchart LR
	ACA[Heavy Atom Partition]
	ACA-->|Connectivity| ACB[Covalent Radius]
Loading
flowchart LR
	ACC[Intersections]
Loading
flowchart LR
	ACD[Nuclear Graph]
	ACD-->|Connectivity| ACE[Covalent Radius]
	ACD-->|Nodes| ACF[Heavy Atom Partition]
	ACF-->|Connectivity| ACG[Covalent Radius]
Loading
flowchart LR
	ACH[Weighted Distance]
Loading

Comment on lines 17 to 18
#include "module/module_class.hpp"
#include "module_manager/module_manager_class.hpp"
Copy link
Member

Choose a reason for hiding this comment

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

should use paths from the include root for files in the include directory.

@ryanmrichard
Copy link
Member

I think the graphs look great!!!

@jlheflin jlheflin marked this pull request as ready for review March 13, 2025 21:12
Copy link
Member

@ryanmrichard ryanmrichard left a comment

Choose a reason for hiding this comment

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

Nice! This is basically ready to merge. Just accept the #include changes (quotes when the path is relative, angle brackets otherwise) and add the namespaces and it's r2g!

@ryanmrichard ryanmrichard merged commit 36c7420 into master Mar 14, 2025
4 checks passed
@ryanmrichard ryanmrichard deleted the mermaid-graph branch March 14, 2025 13:58
@jwaldrop107
Copy link
Member

🚀 [bumpr] Bumped! New version:v1.0.43 Changes:v1.0.42...v1.0.43

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.

4 participants