Skip to content

Conversation

@JainTwinkle
Copy link
Collaborator

@JainTwinkle JainTwinkle commented Jul 4, 2022

The current make install does not install MANA executables and libraries to the installation directory (provided by --prefix).
With this PR, make and make -j install work properly. Though it works. One doesn't have to call make mana to build MANA anymore (make is enough).
Catch: Sometimes, make -j mana install throw errors due to missing dependencies in the DMTCP submodule. For now, one should either use make -j install or use make -j mana && make -j install to install correctly.

Copy link
Collaborator

@karya0 karya0 left a comment

Choose a reason for hiding this comment

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

@JainTwinkle: I have only partially reviewed this PR but we need some significant reversal especially around changes involving DESTDIR variable. This is a user-supplied variable for staged install and we shouldn't change it.

@@ -46,7 +46,7 @@ default: display-build-env add-git-hooks mana_prereqs
all: default

mana: dmtcp
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure why we are doing a make install here. Shouldn't that go under the install target?

Also, should we just remove this target altogether.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@karya0 , I'm not sure I understand your point here. We want to capture the default target for 'make'. The conventional name for that would be default and not install. Under that target, it's possible to call make install if desired. Something like that seems to be happening here.

cd dmtcp && make DESTDIR=$(DESTDIR) $(INSTALL_FLAGS)
cd mpi-proxy-split && make DESTDIR=$(DESTDIR) $(INSTALL_FLAGS)
cd manpages && make DESTDIR=$(DESTDIR) $(INSTALL_FLAGS)
cd mpi-proxy-split && make DESTDIR=$(prefix) $(INSTALL_FLAGS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn't set DESTDIR to prefix. These are two separate variables and typically the final location is $(DESTDIR)$prefix

DMTCP_ROOT=${MANA_ROOT}/dmtcp
endif

ifeq ($(DESTDIR),)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not touch DESTDIR at all -- this is something that's supplied as part of the make command itself.

Copy link
Collaborator

@gc00 gc00 left a comment

Choose a reason for hiding this comment

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

@JainTwinkle and @karya0 , As a general comment, do we still want the user to be able to build DMTCP inside the MANA distribution? Or do we want to hide the build of DMTCP behind abstractions so that the typical user will not be able to build DMTCP within MANA?

I worry that having one distribution for MANA only (can't easily biuld DMTCP) and one distribution for DMTCP could lead eventually to accidental divergence of the two codes. I have no problem if 'make' builds MANA by default. But then, should we also offer make dmtcp, which would build only the DMTCP binaries/libraries?

Or are we assuming that a build of MANA will always build DMTCP correctly for the general DMTCP case, and so there's no need to directly test DMTCP alone?

@@ -46,7 +46,7 @@ default: display-build-env add-git-hooks mana_prereqs
all: default

mana: dmtcp
Copy link
Collaborator

Choose a reason for hiding this comment

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

@karya0 , I'm not sure I understand your point here. We want to capture the default target for 'make'. The conventional name for that would be default and not install. Under that target, it's possible to call make install if desired. Something like that seems to be happening here.

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