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

Revise Mac installation instructions #1918

Merged
merged 21 commits into from
Mar 3, 2021

Conversation

heplesser
Copy link
Contributor

This PR provides revised Mac installation instructions and also includes some additional updates to installation guidelines.

@heplesser heplesser added S: High Should be handled next T: Maintenance Work to keep up the quality of the code and documentation. I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Feb 5, 2021
@heplesser heplesser self-assigned this Feb 5, 2021
Copy link
Contributor

@pnbabu pnbabu left a comment

Choose a reason for hiding this comment

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

Just a few minor changes from my end.

doc/installation/index.rst Outdated Show resolved Hide resolved

#. Extract the NEST tarball as a subdirectory in that directory or clone NEST from GitHub into a subdirectory:
#. Activate the environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is better to provide the command to activate the environment here (in case they don't see the conda_tips page)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@heplesser heplesser requested a review from pnbabu February 8, 2021 14:38
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.

I suggest to remove the INSTALL file altogether, as it just replicates the information presented in the installation guide.

@jougs jougs requested review from jessica-mitchell and removed request for terhorstd and sarakonradi February 25, 2021 23:24
doc/installation/compilation_options.rst Outdated Show resolved Hide resolved
doc/installation/conda_tips.rst Outdated Show resolved Hide resolved
doc/installation/conda_tips.rst Outdated Show resolved Hide resolved
doc/installation/conda_tips.rst Outdated Show resolved Hide resolved
doc/installation/conda_tips.rst Outdated Show resolved Hide resolved
doc/installation/mac_install.rst Outdated Show resolved Hide resolved
doc/installation/mac_install.rst Outdated Show resolved Hide resolved
doc/installation/mac_install.rst Outdated Show resolved Hide resolved
doc/installation/mac_install.rst Outdated Show resolved Hide resolved
doc/installation/mac_install.rst Outdated Show resolved Hide resolved
@heplesser
Copy link
Contributor Author

@jougs Done, I have minimized INSTALL to just a pointer.

Co-authored-by: jessica-mitchell <[email protected]>
@heplesser
Copy link
Contributor Author

@jessica-mitchell Thanks for your suggestions, I accepted them all.

Copy link
Contributor

@jessica-mitchell jessica-mitchell left a comment

Choose a reason for hiding this comment

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

Thanks for the work on this @heplesser, everything looks good to me

@heplesser
Copy link
Contributor Author

@pnbabu Could you check if these instructions work for you?

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.

Very nice! I have added some review comments inline.

Contrary to my first impression, this was not too Mac specific for me.

INSTALL Outdated Show resolved Hide resolved
doc/installation/cmake_options.rst Outdated Show resolved Hide resolved
doc/installation/cmake_options.rst Outdated Show resolved Hide resolved
doc/installation/cmake_options.rst Outdated Show resolved Hide resolved
doc/installation/conda_tips.rst Outdated Show resolved Hide resolved
doc/installation/mac_install.rst Outdated Show resolved Hide resolved
doc/installation/mac_install.rst Outdated Show resolved Hide resolved
doc/installation/mac_install.rst Outdated Show resolved Hide resolved
doc/installation/mac_install.rst Outdated Show resolved Hide resolved
doc/troubleshooting.rst Outdated Show resolved Hide resolved
@jougs jougs removed the request for review from pnbabu February 26, 2021 10:48
@heplesser
Copy link
Contributor Author

@jessica-mitchell @sarakonradi @jougs I now changed all nest-related paths to the nest_source_dir format. I am a bit uncertain about paths given to cmake switches like these (in cmake_options.rst):

-Dwith-music=[OFF|ON|</path/to/music>]
-Dwith-mpi=[OFF|ON|</path/to/mpi>]
-Dwith-gsl=[OFF|ON|</path/to/gsl>]
-Dwith-readline=[OFF|ON|</path/to/readline>]
-Dwith-ltdl=[OFF|ON|</path/to/ltdl>]

They made me wonder the path to precisely what we require here, so I'd leave this unchanged here and make it a separate issue.

Also, the Python flag documentation

-Dwith-python=[OFF|ON]     Build PyNEST. [default=ON]

doesn't even mention the possibility of giving an executable or path; again, not a matter for this PR (see also #1948).

@heplesser
Copy link
Contributor Author

@jougs Now INSTALL is gone for good :).

…nto mac-installation-instructions

Conflicts:
	doc/contribute/development_workflow.rst
	doc/installation/conda_tips.rst
	doc/installation/hpc_install.rst
	doc/installation/index.rst
	doc/installation/linux_install.rst
	doc/installation/mac_install.rst
	doc/troubleshooting.rst
@heplesser
Copy link
Contributor Author

Merging master after #1843 was a bit of an effort, I hope I didn't break anyting ;).

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.

Thanks! I'm fine now.

Regarding the documentation of build flags, I suggest to create another issue/PR to fix that.

@heplesser heplesser merged commit 2a0e81d into nest:master Mar 3, 2021
@heplesser heplesser deleted the mac-installation-instructions branch March 3, 2021 10:33
@terhorstd terhorstd changed the title Revised Mac installation instructions Revise Mac installation instructions Jun 10, 2021
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: High Should be handled next T: Maintenance Work to keep up the quality of the code and documentation.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants