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

Fix #3124 #3178

Merged
merged 1 commit into from Mar 13, 2018
Merged

Fix #3124 #3178

merged 1 commit into from Mar 13, 2018

Conversation

NK-Nikunj
Copy link
Member

@NK-Nikunj NK-Nikunj commented Feb 17, 2018

Fixes #3124

Proposed Changes

  • Added CMakeLists.txt in examples/hello_world_component specific only to external builds. This CMakeLists.txt will not be executed while running cmake for building hpx.
  • Added cmake script in circle.yml to integrate the hello_world_component into CI build.

Edit

  • Fixed a typo in documentation for "using_hpx_pkgconfig".

@hkaiser
Copy link
Member

hkaiser commented Feb 17, 2018

@NK-Nikunj This PR contains the same changes as #3176. Should we close that other one without merging or will you remove that commit from this one?

@NK-Nikunj
Copy link
Member Author

@hkaiser I forgot to see that commit to #3176 is also added here. Since this commit has been added to this one as well, I feel you can close #3176. I will edit the proposed changes above to include the changes done in #3176 as well.

@msimberg
Copy link
Contributor

@NK-Nikunj The hello world component is failing during cmake configuration (you can see the output by clicking on details on the circleci entry). I believe you need to change HPX_DIR to ../build/lib/cmake/HPX in order for HPX to be found correctly (see docs).

@NK-Nikunj
Copy link
Member Author

@msimberg Thank you!

@hkaiser
Copy link
Member

hkaiser commented Mar 4, 2018

@NK-Nikunj have you deleted all of the actual code now? Also, please note that if you rely on pkg-config, this has to be enabled for Linux systems only (or if pkg-config was found by cmake).

@NK-Nikunj
Copy link
Member Author

@hkaiser, @sithhell told me to build hello_world_component to test pkg-config instead of the test code that was given in the src. That's why I deleted the src folder.

@NK-Nikunj NK-Nikunj force-pushed the fix-#3124 branch 3 times, most recently from aa911fa to f49bd16 Compare March 8, 2018 14:06
@hkaiser
Copy link
Member

hkaiser commented Mar 10, 2018

To re-iterate: the purpose of the original example was to demonstrate how an external program depending on HPX can be built. We ran into issues as at some point the example was not compiling anymore. #3124 proposed to solve this by adding the code to the CI system, ensuring that any future changes wouldn't break the example.

Now, this PR not only proposes to build another (unrelated) example, it also proposes to remove the original example source code (sorry if I misunderstand things). Could you please explain your rationale in more detail?

@NK-Nikunj
Copy link
Member Author

NK-Nikunj commented Mar 10, 2018

@hkaiser I think you have misunderstood what I have proposed. When I asked @sithhell for implementation, he told me that the previous example ( the one that was in src directory ) was being built externally. He told me to build the hello_world_component from that build.

What I have done is simply redirect the tests/unit/build/CMakelists.txt to invoke examples/hello_world_component/CMakelists.txt instead of the tests/unit/build/src/CMakelists.txt. Also, I have changed the Makefile accordingly so that hello_world_client can be built. The hello_world_component cmake can also be invoked by a user if he wishes to build hello_world_component by also passing HPX_DIR into cmake command.

Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

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

LGTM now, thanks!

@msimberg msimberg merged commit a281166 into STEllAR-GROUP:master Mar 13, 2018
@NK-Nikunj NK-Nikunj deleted the fix-#3124 branch March 14, 2018 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants