schism-dev / schism

Semi-implicit Cross-scale Hydroscience Integrated System Model (SCHISM)
http://ccrm.vims.edu/schismweb/
Apache License 2.0
87 stars 86 forks source link

leftover comma #46

Closed jbclements closed 2 years ago

jbclements commented 2 years ago

https://github.com/schism-dev/schism/blob/cd8341e8f01aacb866aa44e302f1f748a3ac7a68/mk/Make.defs.ubuntu.gnu.ompi#L43

There definitely should not be a comma after openmpi-bin in this apt-get install command....

josephzhang8 commented 2 years ago

Thx John. Hi Carsten: can u take a look? Thx.

-Joseph

Joseph Zhang Office: (804) 684 7466 Web: schism.wiki

From: John Clements @. Sent: Tuesday, October 12, 2021 7:25 PM To: schism-dev/schism @.> Cc: Subscribed @.***> Subject: [schism-dev/schism] leftover comma (#46)

[EXTERNAL to VIMS received message]

https://github.com/schism-dev/schism/blob/cd8341e8f01aacb866aa44e302f1f748a3ac7a68/mk/Make.defs.ubuntu.gnu.ompi#L43https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fschism-dev%2Fschism%2Fblob%2Fcd8341e8f01aacb866aa44e302f1f748a3ac7a68%2Fmk%2FMake.defs.ubuntu.gnu.ompi%23L43&data=04%7C01%7Cyjzhang%40vims.edu%7C9305f476cf644231983108d98dd77efd%7C8cbcddd9588d4e3b9c1e2367dbdf1740%7C0%7C0%7C637696778980481296%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=KT8YCCofFxFQd9W%2FyklafU5azkjdwPNtOSEIJT8Vli8%3D&reserved=0

There definitely should not be a comma after openmpi-bin in this apt-get install command....

- You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fschism-dev%2Fschism%2Fissues%2F46&data=04%7C01%7Cyjzhang%40vims.edu%7C9305f476cf644231983108d98dd77efd%7C8cbcddd9588d4e3b9c1e2367dbdf1740%7C0%7C0%7C637696778980491253%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=aGAMy76J7g2NGU9nNT%2BE5kD7LJmpgaYrgqVkdakFc78%3D&reserved=0, or unsubscribehttps://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAFBKNZ65ZYC4PBC6QVSGQG3UGS7UJANCNFSM5F33ALWQ&data=04%7C01%7Cyjzhang%40vims.edu%7C9305f476cf644231983108d98dd77efd%7C8cbcddd9588d4e3b9c1e2367dbdf1740%7C0%7C0%7C637696778980501210%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=oM8onCBiBbriknHXq7bIRh8lpd736uonxDxsvZt17Lk%3D&reserved=0. Triage notifications on the go with GitHub Mobile for iOShttps://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fapps.apple.com%2Fapp%2Fapple-store%2Fid1477376905%3Fct%3Dnotification-email%26mt%3D8%26pt%3D524675&data=04%7C01%7Cyjzhang%40vims.edu%7C9305f476cf644231983108d98dd77efd%7C8cbcddd9588d4e3b9c1e2367dbdf1740%7C0%7C0%7C637696778980501210%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=%2F6Y9zHc%2BoxikC81JdUeUUe1Kou8f0g2fspJlg8Ps5yE%3D&reserved=0 or Androidhttps://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fplay.google.com%2Fstore%2Fapps%2Fdetails%3Fid%3Dcom.github.android%26referrer%3Dutm_campaign%253Dnotification-email%2526utm_medium%253Demail%2526utm_source%253Dgithub&data=04%7C01%7Cyjzhang%40vims.edu%7C9305f476cf644231983108d98dd77efd%7C8cbcddd9588d4e3b9c1e2367dbdf1740%7C0%7C0%7C637696778980511164%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=N%2Bkm5JuUBXaQhL0xEzzLctWdNc28CUR1gY271RE0HGY%3D&reserved=0.

platipodium commented 2 years ago

Yes, the comma is wrong here. But moreover, this makefile snippet is using outdated concepts:

1) We can assume now that everybody is on a modern netcdf, where the C and Fortran libs are separate (this is explained only later in the this snippet, but should be made default 2) The infrastructure needed to set the environment variables is now provided through nc-config almost completely. 3) Makefile infrastructure has been superseded by CMake infrastructure.

@jbclements Do you want to go ahead, fix the issue and file a PR for us to merge? Otherwise, I'll take this on some time later

jbclements commented 2 years ago

Good to hear from you.

I did install the libnetcdff-dev library, as suggested several lines later. I was also slightly queasy about the fact that nothing past 18.04 was mentioned.

The docs suggest that one can build using either make or cmake; I attempted the make using just plain make. The ParMetis build halted though, indicating that cmake was required (and also g++, but who’s counting). The build then… succeeded? but … it appears that the final makefile target is libschism.a.

I’m getting the sense that it was a mistake to try building from the first set of build instructions (the ones using make), and that I should simply have skipped that and gone straight to cmake.

If that succeeds, I can make a pull request, but it seems like the right pull request might just be to rip out the mk directory entirely, and trim the docs to instruct people to go straight for cmake.

Thanks for your response!

John Clements

On Oct 12, 2021, at 21:15, Carsten Lemmen @.***> wrote:

Yes, the comma is wrong here. But moreover, this makefile snippet is using outdated concepts:

• We can assume now that everybody is on a modern netcdf, where the C and Fortran libs are separate (this is explained only later in the this snippet, but should be made default • The infrastructure needed to set the environment variables is now provided through nc-config almost completely. • Makefile infrastructure has been superseded by CMake infrastructure. @jbclements Do you want to go ahead, fix the issue and file a PR for us to merge? Otherwise, I'll take this on some time later

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

jbclements commented 2 years ago

Okay, success. Well, I have an executable, at any rate. My instructions were to get it compiled, now I hand it off to someone who knows something about fluids :).

I definitely felt quite a bit more at sea in the cmake build, there was no config file that obviously matched an ubuntu build, but the whirlwind.gnu cmake file worked for me. I did paste in the absolute paths to the netcdf shared libraries.

Here’s the text of the SCHISM.local.yikes file that I created, and that I would expect to work for any x86_64 ubuntu 20.04 install (after installing all of the dependencies, of course).

###W&M Whirlwind cluster
#  Set the base name of the executable.
#  The main reason for this is to include something like a cluster/architecture name.
#  Do not add the file extension (none for linux, .exe for Windows etc)
#  or the list of enabled modules, both of which will be automatically appended.
  set (SCHISM_EXE_BASENAME pschism_GNU_WW CACHE STRING "Base name (modules and file extension to be added of the executable. If you want a machine name, add it here")

###Relative paths won't work
set(CMAKE_Fortran_COMPILER gfortran CACHE PATH "Path to serial Fortran compiler")
set(CMAKE_C_COMPILER gcc CACHE PATH "Path to serial C compiler")
set(NetCDF_FORTRAN_DIR /usr/lib/x86_64-linux-gnu/libnetcdff.so CACHE PATH "Path to NetCDF Fortran library")
set(NetCDF_C_DIR  /usr/lib/x86_64-linux-gnu/libnetcdf.so  CACHE PATH "Path to NetCDF C library")

###Compile flags
#CMAKE_EXE_LINKER_FLAGS did not work so I had to remove -static
set(CMAKE_Fortran_FLAGS_RELEASE "-O2 -ffree-line-length-none -static-libgfortran -finit-local-zero" CACHE STRING "Fortran flags" FORCE)

As I said before, I think one simple update to the docs would be to slice out the section that suggests that you can build without using cmake. Or maybe the manual has changed in git HEAD? (goes and checks) Oh… doesn’t look like the manual is part of that repo? Ah well.

I’m happy to make a pull request to add this file (with a bit of cleanup and commenting) as a SCHISM.local.ubuntu, if you think it would be helpful.

Many thanks,

John Clements

On Oct 13, 2021, at 00:08, John Clements @.***> wrote:

Good to hear from you.

I did install the libnetcdff-dev library, as suggested several lines later. I was also slightly queasy about the fact that nothing past 18.04 was mentioned.

The docs suggest that one can build using either make or cmake; I attempted the make using just plain make. The ParMetis build halted though, indicating that cmake was required (and also g++, but who’s counting). The build then… succeeded? but … it appears that the final makefile target is libschism.a.

I’m getting the sense that it was a mistake to try building from the first set of build instructions (the ones using make), and that I should simply have skipped that and gone straight to cmake.

If that succeeds, I can make a pull request, but it seems like the right pull request might just be to rip out the mk directory entirely, and trim the docs to instruct people to go straight for cmake.

Thanks for your response!

John Clements

On Oct 12, 2021, at 21:15, Carsten Lemmen @.***> wrote:

Yes, the comma is wrong here. But moreover, this makefile snippet is using outdated concepts:

• We can assume now that everybody is on a modern netcdf, where the C and Fortran libs are separate (this is explained only later in the this snippet, but should be made default • The infrastructure needed to set the environment variables is now provided through nc-config almost completely. • Makefile infrastructure has been superseded by CMake infrastructure. @jbclements Do you want to go ahead, fix the issue and file a PR for us to merge? Otherwise, I'll take this on some time later

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.

platipodium commented 2 years ago

@jbclements please remove the references to NetCDF, this should be auto configured by CMake, what in the end remains is merely

set(CMAKE_Fortran_COMPILER gfortran CACHE PATH "Path to serial Fortran compiler")
set(CMAKE_C_COMPILER gcc CACHE PATH "Path to serial C compiler")
set(CMAKE_Fortran_FLAGS_RELEASE "-O2 -ffree-line-length-none -static-libgfortran -finit-local-zero" CACHE STRING "Fortran flags" FORCE)

which to me seems pretty generic for any gnu system. What is specific to Ubuntu is the enumeration of needed packages (different between distros and package managers). Also, the update-alternatives mechanism to select the MPI to be used.

jbclements commented 2 years ago

Made pull request, took a pass at trying to write a set of install steps for Ubuntu, not sure where those should go (I put them in the comment of the pull request)

jbclements commented 2 years ago

Also, I'm certainly not going to make wholesale removal of the "mk" directory part of this pull request, that's way above my pay grade.

jbclements commented 2 years ago

Sure enough, my instructions for ubuntu install were incomplete.


What follows are the steps that appear to work for me on a fresh install of 
ubuntu 20.24.

* Start with fresh Ubuntu 20.04.

* Create a new user with sudo privileges, su to that user.

* Install a bunch of packages:

> sudo apt install gcc g++ gfortran libnetcdf15 openmpi-bin libnetcdf-dev libnetcdff-dev libopenmpi-dev openmpi-bin python2 cmake

* Next, the world has moved on and python 2 is no longer standard; the "update-alternatives"
  utility can create a link that causes python to invoke python v2.7:

> sudo update-alternatives --install /usr/local/bin/python python /usr/bin/python2 20

* Now, check out the schism repo:

> cd ~
> git clone https://github.com/schism-dev/schism.git
> cd schism/
> git checkout tags/v5.9.0
> cd cmake

* create a file in the cmake directory called SCHISM.local.ubuntu, containing this text:

* * *
set(CMAKE_Fortran_COMPILER gfortran CACHE PATH "Path to serial Fortran compiler")
set(CMAKE_C_COMPILER gcc CACHE PATH "Path to serial C compiler")
set(CMAKE_Fortran_FLAGS_RELEASE "-O2 -ffree-line-length-none -static-libgfortran -finit-local-zero" CACHE STRING "Fortran flags" FORCE)
* * *

(Don't put the asterisks in there, those are just there to demarcate the
beginning and end of the file.)

(NB if my pull request is accepted upstream, this file may become
built-in.)

* Finally, run cmake and make:

> cd ~/schism/
> mkdir build
> cd build
> cmake -C ../cmake/SCHISM.local.build -C ../cmake/SCHISM.local.ubuntu ../src/
> make pschism

After this, the binary (just called pschism, I believe) should appear in
the bin/ subdirectory, and the libraries in ... another subdirectory,
mumble, let me know if you can't find them.
jamal919 commented 2 years ago

A small note though: Setting the NETCDF related variables by hand is quite useful if someone uses conda/anaconda for their python distribution. Conda/Anaconda comes with its own distribution of the netcdf and often CMake script picks up mixed NetCDF /include directory and /lib directory. Hence, I would recommend to mention it somewhere in the installation instruction.

@jbclements please remove the references to NetCDF, this should be auto configured by CMake, what in the end remains is merely

set(CMAKE_Fortran_COMPILER gfortran CACHE PATH "Path to serial Fortran compiler")
set(CMAKE_C_COMPILER gcc CACHE PATH "Path to serial C compiler")
set(CMAKE_Fortran_FLAGS_RELEASE "-O2 -ffree-line-length-none -static-libgfortran -finit-local-zero" CACHE STRING "Fortran flags" FORCE)

which to me seems pretty generic for any gnu system. What is specific to Ubuntu is the enumeration of needed packages (different between distros and package managers). Also, the update-alternatives mechanism to select the MPI to be used.