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 'install-nodoc' target for make #890

Merged
merged 9 commits into from Apr 5, 2018

Conversation

@jougs
Copy link
Contributor

@jougs jougs commented Feb 6, 2018

This fixes #153.

Unfortunately, CMake does not support custom code or conditionals in the install target. I therefore implemented this as an extension to the help extraction system itself.

@jougs jougs requested review from heplesser and steffengraber Feb 6, 2018
Copy link
Contributor

@heplesser heplesser left a comment

@jougs Thanks for fixing this. Could you add a line to the INSTALL file documenting this possibility and have a look at the one questions I put into the code?

@@ -361,4 +361,8 @@ install( FILES LICENSE README.md NEWS
DESTINATION ${CMAKE_INSTALL_DOCDIR}
)

add_custom_target( install-nodoc
COMMAND make NEST_INSTALL_NODOC=true install

This comment has been minimized.

@heplesser

heplesser Feb 15, 2018
Contributor

Can I still use -j N with this?

This comment has been minimized.

@jougs

jougs Feb 19, 2018
Author Contributor

The short answer is yes.

The slightly longer answer is that my changes do not alter the behavior of your call to make for any target but install-nodoc. And for install -j N did not have an effect anyway as the installation is always performed serially in order to avoid problems with concurrent writes.

@jougs
Copy link
Contributor Author

@jougs jougs commented Feb 19, 2018

@heplesser: Thanks for the review. I've addressed your comments in my commit
2b16a06 and my reply to your question.

@terhorstd terhorstd added this to the NEST 2.16 milestone Mar 5, 2018
@heplesser
Copy link
Contributor

@heplesser heplesser commented Mar 5, 2018

@jougs I can use the install-nodoc target, but it still installs the help.

@heplesser
Copy link
Contributor

@heplesser heplesser commented Mar 5, 2018

@jougs I just sent you a PR adding a required "optional" flag. With this, things work in a way: as long as one only ever calls make with the install-nodoc, help will not get installed. If invoking a "naked" make, doc/help will be populated and then installed in all subsequent make invocations, even for target install-nodoc.

I wonder if a complete solution of this problem would require introduction of proper build and install configurations, such as deploy/develop/debug.

jougs added 2 commits Apr 4, 2018
This makes the install-nodoc target work by first deleting the help directory,
creating help for the install target, install the help directory if it was
created, and generating the help index for the installed help directory if that
exists.
@jougs
Copy link
Contributor Author

@jougs jougs commented Apr 5, 2018

@heplesser: I've come up with a reasonably stable solution in my recent commits. Can you please test again? Thanks!

Copy link
Contributor

@heplesser heplesser left a comment

@jougs Great, thanks! I works! I just have two little details, see comments in the code.

@@ -152,6 +152,10 @@ def write_helpindex(helpdir):
---------------------------------------
"""

if not os.path.exists(helpdir):

This comment has been minimized.

@heplesser

heplesser Apr 5, 2018
Contributor

Please add a comment explaining why just returning is fine instead of raising an error. In the future, we may not remember the logic behind the return here.

)

# Update the global help index to include all help files in
# the global installation directory for

This comment has been minimized.

@heplesser

heplesser Apr 5, 2018
Contributor

Fix indentation.

Copy link
Collaborator

@steffengraber steffengraber left a comment

Perfect, Works for me.

@heplesser heplesser merged commit fb3414a into nest:master Apr 5, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants