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

Added dependency resolution to make files. #29

Merged
merged 3 commits into from
Nov 3, 2015
Merged

Conversation

rbooth200
Copy link
Collaborator

Add header dependency resolution by generating extra rules using gcc's header resolution options.

endif

CPP_TGT := $(patsubst %.o, %.cpp, $(OBJ))
CPP_TGT := $(patsubst chealpix.cpp, , $(CPP_TGT))
Copy link
Contributor

Choose a reason for hiding this comment

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

What does healpix have to do with this problem? Is that name just a placeholder?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

patsubstr is using the list of objects to generate a .cpp file name. The dependency resolution checks to see if these files 1) exist and 2) have been modified. Since chealpix is a .c file, Make obviously won't find chealpix.cpp as it doesn't exist. Therefore, I just use patsubstr to remove it from the list.

It would be neater to create a list of .cpp files and .c files that I could use for dependency resolution, from which the .o file list is created. But this method works and required a much smaller change. Maybe the best thing is to just add a comment for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, fine. Well, a simple search tells me that chealpix is the only .c file. So it's probably fine for now (unless we're going to add more .c files). I'll test after lunch if the dependencies work also on the mac

@giovanni-rosotti
Copy link
Contributor

Ok, works for me on the mac too. Two last points before merging (I'm being very picky just because the makefile is such an important part of the interface with the user):
The dependency files contain also system header files - is that we want? I suppose it shouldn't be a problem, just make will take a bit longer because it needs to check also those timestamps (hardly a problem).
I wonder if we should use the CPP variable rather than g++ (but checking if g++ is part of the name) in case the user does not have a command called g++. Or maybe we should just add a warning in this case informing the user that we were not able to compute the dependencies.

@rbooth200
Copy link
Collaborator Author

In which case, replace -M with -MM

On 02/11/15 13:01, giovanni-rosotti wrote:

Ok, works for me on the mac too. Two last points before merging (I'm
being very picky just because the makefile is such an important part
of the interface with the user):
The dependency files contain also system header files - is that we
want? I suppose it shouldn't be a problem, just make will take a bit
longer because it needs to check also those timestamps (hardly a problem).
I wonder if we should use the CPP variable rather than g++ (but
checking if g++ is part of the name) in case the user does not have a
command called g++. Or maybe we should just add a warning in this case
informing the user that we were not able to compute the dependencies.


Reply to this email directly or view it on GitHub
#29 (comment).

@rbooth200
Copy link
Collaborator Author

It only reports files included using "" and not <>

On 02/11/15 13:01, giovanni-rosotti wrote:

Ok, works for me on the mac too. Two last points before merging (I'm
being very picky just because the makefile is such an important part
of the interface with the user):
The dependency files contain also system header files - is that we
want? I suppose it shouldn't be a problem, just make will take a bit
longer because it needs to check also those timestamps (hardly a problem).
I wonder if we should use the CPP variable rather than g++ (but
checking if g++ is part of the name) in case the user does not have a
command called g++. Or maybe we should just add a warning in this case
informing the user that we were not able to compute the dependencies.


Reply to this email directly or view it on GitHub
#29 (comment).

giovanni-rosotti added a commit that referenced this pull request Nov 3, 2015
Added dependency resolution to make files.
@giovanni-rosotti giovanni-rosotti merged commit 340ce98 into master Nov 3, 2015
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.

2 participants