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 information about nest_vars.sh and update Linux installation instructions #1515

Merged
merged 29 commits into from
Jun 16, 2020

Conversation

sarakonradi
Copy link
Contributor

This PR adds information on setting the module loading path correctly.

More specifically, it:

  • Adds a Setting the module loading path section below Installation from source
  • Removes the duplicate information in the IMPORTANT! admonition
  • Turns these instructions into numbered steps for a better user experience

It fixes #1222.

@sarakonradi sarakonradi requested a review from jougs April 15, 2020 09:53
@sarakonradi sarakonradi mentioned this pull request Apr 15, 2020
24 tasks
@sarakonradi
Copy link
Contributor Author

@jougs Does this address your concern? Or do you suggest adding a section about nest_vars.sh in general, explaining all the paths that it entails?

@sarakonradi sarakonradi self-assigned this Apr 15, 2020
@sarakonradi sarakonradi changed the title Add information on setting NEST_MODULE_PATH by sourcing nest_vars.sh Add information about nest_vars.sh to documentation Apr 15, 2020
@heplesser heplesser requested review from heplesser, clinssen and terhorstd and removed request for jougs and heplesser April 20, 2020 08:47
@heplesser heplesser added I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation labels Apr 20, 2020
@heplesser heplesser added this to the NEST 3.0 milestone Apr 20, 2020
Copy link
Contributor

@terhorstd terhorstd left a comment

Choose a reason for hiding this comment

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

added a little suggestion, but mostly fine with me. I would accept this with the changes.

I also like the idea of @sarakonradi to have a separate page explaining nest_vars.sh, that could be about general environment handling containing also the cases where nest_vars.sh is not needed when installing into a $CONDA_PREFIX or when it is replaced by a module load type of setup, etc. That should be a separate PR, however.

doc/installation/linux_install.rst Outdated Show resolved Hide resolved
@clinssen
Copy link
Contributor

@sarakonradi: could you set this PR to draft mode?

Waiting for my PR on @sarakonradi's fork to be merged (see https://github.com/sarakonradi/nest-simulator/pull/2/files) following updates in #1513.

Please feel free to leave any comments/suggestions there!

@sarakonradi sarakonradi marked this pull request as draft April 22, 2020 09:14
@sarakonradi
Copy link
Contributor Author

@clinssen Sure! Just set it to draft mode.

It seems that your PR still includes the old information. For example, Python has been changed to Python 3.X and NEST requires at least version v2.8.12 of cmake has been updated to NEST requires CMake 3.12 or higher: https://github.com/nest/nest-simulator/blob/master/doc/installation/linux_install.rst

Could you perhaps merge master again and then let me know?

@sarakonradi
Copy link
Contributor Author

@clinssen Thanks for rebasing. Just merged your PR and will keep an eye on #1513.

@sarakonradi
Copy link
Contributor Author

Very nice, thanks! I still have some suggestions for you to consider.

Thanks for your suggestions! Just applied them. I also fixed the merge conflict. doc/installation/linux_install.rst now includes the note on python bindings being enabled by default (#1507).

@jougs @terhorstd Please check!

Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@clinssen clinssen left a comment

Choose a reason for hiding this comment

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

Many thanks!

@jougs
Copy link
Contributor

jougs commented May 7, 2020

@terhorstd are you also happy? If yes, please also click the button. Thanks!

@sarakonradi
Copy link
Contributor Author

@terhorstd I created a separate issue (#1612) to address your concern regarding the dependencies file.

Copy link
Contributor

@terhorstd terhorstd left a comment

Choose a reason for hiding this comment

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

This PR does a lot more changes than adding information about nest_vars.sh, it basically reworks the Linux install instructions.

doc/installation/linux_install.rst Show resolved Hide resolved
Co-authored-by: terhorstd <[email protected]>
@sarakonradi
Copy link
Contributor Author

Added the information on Boost, as suggested by @terhorstd.

This PR does a lot more changes than adding information about nest_vars.sh, it basically reworks the Linux install instructions.

This is a classic instance of a PR evolving (more than it should) over time. As discussed in the doc meeting, it is best to keep future PRs small! Should we rename it or keep the title?

@clinssen
Copy link
Contributor

Would propose to update the title and merge this so we can move on.

@sarakonradi sarakonradi changed the title Add information about nest_vars.sh to documentation Add information about nest_vars.sh and update Linux installation instructions Jun 15, 2020
@sarakonradi
Copy link
Contributor Author

Would propose to update the title and merge this so we can move on.

Done.

@jougs jougs removed the request for review from terhorstd June 15, 2020 20:15
@jougs
Copy link
Contributor

jougs commented Jun 15, 2020

I totally agree that we should merge this one now and suggest to mark up the remarks in a next step, which should probably also check if they are still valid, some of them seem quite outdated...

@terhorstd: can you have a final look and push the button if you're happy?

@terhorstd terhorstd merged commit b284961 into nest:master Jun 16, 2020
@sarakonradi sarakonradi deleted the fix_issue_1222 branch September 21, 2020 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Enhancement New functionality, model or documentation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add LD_LIBRARY_PATH to guide for "writing an extension module"
5 participants