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

Remove default petsc path in the Makefile and istallation #55

Merged
merged 13 commits into from
Mar 2, 2022

Conversation

aguspesce
Copy link
Member

@aguspesce aguspesce commented Feb 7, 2022

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

  • Remove the default path for pests at ~
  • Add instruction to add a symlinks of mpirun
  • Add instruction to create an env variable for PETSC_DIR before build Mandyoc

@aguspesce
Copy link
Member Author

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

@aguspesce aguspesce requested review from jamisonassuncao, rafaelmds and victorsacek and removed request for jamisonassuncao February 7, 2022 14:35
Makefile Outdated Show resolved Hide resolved
@rafaelmds rafaelmds changed the title Remove default petsc path in the Makefile and istallation WIP - Remove default petsc path in the Makefile and istallation Feb 8, 2022
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
aguspesce and others added 2 commits February 10, 2022 09:39
Co-authored-by: Rafael Silva <rafaelmds@users.noreply.github.com>
Co-authored-by: Rafael Silva <rafaelmds@users.noreply.github.com>
@rafaelmds rafaelmds mentioned this pull request Feb 11, 2022
2 tasks
@rafaelmds
Copy link
Contributor

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

  • I had some problems with the make install command, so I've tweaked it a bit by adding the file name and making sure the dest dir exists. Before, it was creating the mandyoc binary as ~/.local/bin.
  • added clean to override petsc default rule
  • added check for PETSC_DIR

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
Copy link
Member Author

@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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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

@rafaelmds
Copy link
Contributor

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

@rafaelmds
Copy link
Contributor

@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..."

Add a note about the ~/.local/bin folder

Co-authored-by: Rafael Silva <rafaelmds@users.noreply.github.com>
@aguspesce
Copy link
Member Author

@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
Copy link
Contributor

Thats fine!

@rafaelmds
Copy link
Contributor

I think we can merge, right? @aguspesce

@aguspesce
Copy link
Member Author

@rafaelmds, I think that it is ok!

@rafaelmds rafaelmds changed the title WIP - Remove default petsc path in the Makefile and istallation Remove default petsc path in the Makefile and istallation Mar 2, 2022
@rafaelmds rafaelmds merged commit 0f574f4 into main Mar 2, 2022
@rafaelmds rafaelmds deleted the makefile-improved branch March 2, 2022 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants