ggciag / mandyoc

MANDYOC is a finite element code written on top of the PETSc library to simulate thermo-chemical convection of the Earth's mantle
https://ggciag.github.io/mandyoc/
BSD 3-Clause "New" or "Revised" License
27 stars 5 forks source link

Remove default petsc path in the Makefile and istallation #55

Closed aguspesce closed 2 years ago

aguspesce commented 2 years ago

In the Makefile and installation instruction, I removed the default path to $HOME/petsc.

aguspesce commented 2 years ago

@victorsacek and @rafaelmds. Maybe this PR can be a starting point to fix part of #46 and #52. What do you think?

rafaelmds commented 2 years ago

@aguspesce, here some suggestions for Makefile file after some testing:

It's working fine on my environment (with either PETSC_ARCH set or unset). Will test on docker that is using petsc with "prefix type" installation.

I found it easier to attach a patch (instead of using the github suggestion), so you can apply and test it as well.

diff --git a/Makefile b/Makefile
index e415590..37ab8e1 100644
--- a/Makefile
+++ b/Makefile
@@ -1,3 +1,7 @@
+ifndef PETSC_DIR
+$(error PETSC_DIR environment variable is not set. Please, set/export it before continue.)
+endif
+
 include ${PETSC_DIR}/lib/petsc/conf/variables
 include ${PETSC_DIR}/lib/petsc/conf/rules

@@ -23,25 +27,25 @@ BIN_DIR = bin
 MANDYOC = $(BIN_DIR)/mandyoc
 LOCAL_BIN = $(HOME)/.local/bin

-.PHONY: help all install clear test
+.PHONY: help all install clear clean test

 help:
    @echo ""
    @echo "Commands:"
    @echo ""
-   @echo "  all        Build Mandyoc"
+   @echo "  all        Build Mandyoc."
    @echo "  install    Install MANDYOC in ~/.local/bin/"
    @echo "  test       Run the MANDYOC tests using 1 core. It takes several minutes."
-   @echo "  clear      Removes the files produced when building MANDYOC"
+   @echo "  clear      Removes the files produced when building MANDYOC."
    @echo ""

 all: $(MANDYOC)
    @echo ""
    @echo "Mandyoc built in bin/"

-install: $(MANDYOC)
+install: $(MANDYOC) | $(LOCAL_BIN)
    @echo ""
-   install $< $(LOCAL_BIN)
+   install $< $(LOCAL_BIN)/mandyoc

 test:
    @echo "Run MANDYOC test may take several minutes..."
@@ -49,14 +53,16 @@ test:
    pytest -v test/testing_result.py

 clear:
-   rm $(SRC)/*.o
+   rm -f $(SRC)/*.o
    rm -rf $(BIN_DIR)

+clean:: clear
+
 %.o: %.cpp
    ${PCC} -Wall -fdiagnostics-color -c $< -o $@ ${INCFLAGS}

-$(BIN_DIR):
-   mkdir $@
+$(BIN_DIR) $(LOCAL_BIN):
+   mkdir -p $@

 $(MANDYOC): ${OBJECTS} | $(BIN_DIR)
    -${CLINKER} -o $@ ${OBJECTS} ${PETSC_LIB}
aguspesce commented 2 years ago

@rafaelmds I added your suggestion and improve the names of the directory variables:

PREFIX = $(HOME)/.local
BINDIR = $(PREFIX)/bin
BUILDDIR = bin
MANDYOC = $(BUILDDIR)/mandyoc

But I didn't add this line:

$(BIN_DIR) $(LOCAL_BIN):
  mkdir -p $@

Because it is not correct for Makefile to create a folder on the user's system. If the user doesn't have the ~/.local/bin it's a good idea to create it. We need to add all this information about the ~/.local/bin and the PETSC_SIR variable in the documentation

rafaelmds commented 2 years ago

I agree @aguspesce, best not to automatically/programmactically create that folder.

But I didn't add this line:

$(BIN_DIR) $(LOCAL_BIN):
  mkdir -p $@

Because it is not correct for Makefile to create a folder on the user's system. If the user doesn't have the ~/.local/bin it's a good idea to create it. We need to add all this information about the ~/.local/bin and the PETSC_SIR variable in the documentation

If user doesn't have ~/.local/bin dir, this is the error:

make: *** No rule to make target '/home/rafael/.local/bin', needed by 'install'.  Stop.

This is because $(BINDIR) appears in rule requirements:

install: $(MANDYOC) | $(BINDIR)

If we remove $(BINDIR), I think the error points better to the problem:

install bin/mandyoc /home/rafael/.local/bin/mandyoc
install: cannot create regular file '/home/rafael/.local/bin/mandyoc': No such file or directory
make: *** [Makefile:47: install] Error 1
aguspesce commented 2 years ago

Ok @rafaelmds! Now I understand the situation.

I will remove BINDIR as a requirement, so the code will be:

install: $(MANDYOC):
     install $< $(BINDIR)/mandyoc
rafaelmds commented 2 years ago

I noticed there is no entry for src/*.o in .gitignore file. I think we could add it in this PR as well.

rafaelmds commented 2 years ago

@jamisonassuncao We are almost ready with this new Makefile. Could you test on macos and give your feedback?

rafaelmds commented 2 years ago

@aguspesce

I tested on macos as well, and just have the "problem" with make install due missing ~/.local/bin folder.

I suggest the following modification to print a message to the user to create the folder.

diff --git a/Makefile b/Makefile
index 70b8504..1afd806 100644
--- a/Makefile
+++ b/Makefile
@@ -44,7 +44,12 @@ help:
 all: $(MANDYOC)

 install: $(MANDYOC)
+ifeq ($(shell [ -d $(BINDIR) ] && echo 1 || echo 0 ), 1)
    install $< $(BINDIR)/mandyoc
+else
+   @echo -e "\nDirectory \"$(BINDIR)\" does not exists."
+   @echo -e "Please, create this directory before continue.\n"
+endif

 test:
    @echo "Run MANDYOC test may take several minutes..."
aguspesce commented 2 years ago

@aguspesce

I tested on macos as well, and just have the "problem" with make install due missing ~/.local/bin folder.

I suggest the following modification to print a message to the user to create the folder.

diff --git a/Makefile b/Makefile
index 70b8504..1afd806 100644
--- a/Makefile
+++ b/Makefile
@@ -44,7 +44,12 @@ help:
 all: $(MANDYOC)

 install: $(MANDYOC)
+ifeq ($(shell [ -d $(BINDIR) ] && echo 1 || echo 0 ), 1)
  install $< $(BINDIR)/mandyoc
+else
+ @echo -e "\nDirectory \"$(BINDIR)\" does not exists."
+ @echo -e "Please, create this directory before continue.\n"
+endif

 test:
  @echo "Run MANDYOC test may take several minutes..."

I understand the problem, but create the founder is not the solution because you need to add it to the PATH. So maybe it is better to keep it simple. What do you think about this:

  1. Add a variable in the Makefile with the install path (say INSTALL_PATH)
  2. Set it to a default location in the repo, like ~/.local/bin
  3. Tell users to set the location they want when installing by running make INSTALL_PATH="~/bin" install.
rafaelmds commented 2 years ago

Thats fine!

rafaelmds commented 2 years ago

I think we can merge, right? @aguspesce

aguspesce commented 2 years ago

@rafaelmds, I think that it is ok!