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

Add a simple Makefile for Gammapy. #289

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@mapazarr
Member

mapazarr commented Jun 13, 2015

@cdeil Following our discussion in PR #285 here:
#285 (comment)
I created a Makefile that I'm using now. If you think it could be helpful for other people, you can merge it. Otherwise we can close the PR.

@cdeil cdeil added the feature label Jun 14, 2015

@cdeil cdeil added this to the 0.4 milestone Jun 14, 2015

@cdeil cdeil self-assigned this Jun 14, 2015

@cdeil

This comment has been minimized.

Member

cdeil commented Jun 14, 2015

@mapazarr - Thanks for the PR.

As I wrote here I think adding a Makefile to Gammapy might not be a great idea:

At least for now we prefer not to add a Makefile to Gammapy, because that splits the developers into those that use setup.py and those that use make, which can grow into an overall more complicated system where some things are possible only with setup.py and others only with make.

The only make target you define here that's not directly available via setup.py is clean.
Prompted by this PR I just now had a look and I think actually there are setup.py commands to clean the build and docs folders:

python setup.py clean
python setup.py build_sphinx --clean-docs

It's very rare that people produce coverage reports, and for those expert users I think having to run rm htmlcov is OK.

So at the very least the info given here should be updated to mention those setup.py clean commands:
https://gammapy.readthedocs.org/en/latest/development/index.html#how-to-clean-up-old-files

That said, some Python projects do have Makefiles: scikit-learn, scikit-image, rootpy.
(many others don't, e.g. Astropy, Numpy, Scipy, ...)

And there are things which aren't available via setup.py (I think) and aren't very easy to add (I think) and would be useful, e.g.

# remove all untracked files and directories
clean-repo:
    @git clean -f -x -d
clean:
    find . -name "*.so" -o -name "*.pyc" -o -name "*.pyx.md5" | xargs rm -f
trailing-spaces:
    find sklearn -name "*.py" -exec perl -pi -e 's/[ \t]*$$//' {} \;
cython:
    find sklearn -name "*.pyx" -exec $(CYTHON) {} \;
code-analysis:
    flake8 sklearn | grep -v __init__ | grep -v external
    pylint -E -i y sklearn/ -d E1103,E0611,E1101

(these are copied targets from the other Makefiles mentioned, they'd have to be adapted for Gammapy).

There could be other more complicated things, like building conda packages or testing that all the Gammapy example notebooks still run without error ...

So after thinking about this some more while writing this response, I do think a Makefile would be good to have for things that aren't available via setup.py. Whether it's useful to duplicate the most common functionality of setup.py (build, install, test, build_sphinx) and whether this should be documented / recommended to devs or users, I'm not sure.

Before we decide anything and you put more work into this: @adonath @kingj90 - thoughts?

@cdeil cdeil modified the milestones: 0.3, 0.4 Jul 22, 2015

@cdeil cdeil assigned mapazarr and unassigned cdeil Jul 22, 2015

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 22, 2015

@mapazarr – I've changed my mind on this ... let's do it.

Could you please try to make the Makefile better taking my comments above into account.

One thing I like is to have a help target as the first target, like I did it here:
https://github.com/cta-observatory/ctapipe/blob/master/Makefile
It means devs / users don't have to remember any commands, they can just see the most common ones by typing make.

I do not like forwarding any commands from make to setup.py ... I've tried to use this for ctapipe and I think it's too confusing when the user mistypes some command or uses one that doesn't exist:
https://github.com/cta-observatory/ctapipe/blob/master/Makefile#L46

I'll then give this a final review next week or make some tweaks and we'll include this in the 0.3 release.

There should be a changelog entry and the existence of this Makefile and a few examples / comments should be added to the Gammapy docs.
See https://cta-observatory.github.io/ctapipe/getting_started/index.html#getting-started as an example.

Makefile Outdated
@@ -0,0 +1,20 @@
MODULE = gammapy
CURDIR = $(shell pwd)

This comment has been minimized.

@cdeil

cdeil Jul 22, 2015

Member

I think this can be removed and the Makefile will just work the same?
(see the one in ctapipe)

This comment has been minimized.

@mapazarr
Makefile Outdated
CURDIR = $(shell pwd)
build:

This comment has been minimized.

@cdeil

cdeil Jul 22, 2015

Member

Now this is confusing. Why is make build equivalent to python setup.py develop and not python setup.py build?
I'd say, let's just stick to the commands as they are in setup.py and make this consistent, even if their names are a bit cryptic at times.

This comment has been minimized.

@mapazarr

mapazarr Jul 23, 2015

Member

I have removed some targets: build, doc, test, since you mentioned it is not useful to have targets that are wrappers to setup.py. We can add them back. I think it'd be nice to have a few common ones because:

  1. TAB completion for targets works nice with makefiles in not too old shells, but it doesn't with setup.py.
  2. we can define an all target to run develop, test and doc at the same time (similar to what I had before).
    What shall we do?
Makefile Outdated
doc:
python $(CURDIR)/setup.py build_sphinx
all: build test doc

This comment has been minimized.

@cdeil

cdeil Jul 22, 2015

Member

build is always run before test and doc, it's implicit and can be removed here.

This comment has been minimized.

@mapazarr

mapazarr Jul 23, 2015

Member

OK. I removed the methods anyway, as mentioned above.
For the records, build here means run python setup.py develop, which I guess it's not automatically run, when calling test or doc?

@mapazarr mapazarr changed the title from Added a simple Makefile for Gammapy. to Add a simple Makefile for Gammapy. Jul 23, 2015

@mapazarr mapazarr force-pushed the mapazarr:makefile branch from 98eee49 to 8de2a6a Jul 23, 2015

@mapazarr

This comment has been minimized.

Member

mapazarr commented Jul 23, 2015

Ok, I implemented your suggestions. Please have a look.

clean:
rm -rf build docs/_build docs/api htmlcov
# TODO: do we need to clean CYTHON stuff???!!!

This comment has been minimized.

@mapazarr

mapazarr Jul 23, 2015

Member

@cdeil @adonath Suggestions about this TODO? I have never used cython, so I'm not sure where does it generate the files.

@cdeil cdeil assigned cdeil and unassigned mapazarr Jul 29, 2015

@cdeil

This comment has been minimized.

Member

cdeil commented Aug 2, 2015

Oh no ... I rebased this locally to fix the merge conflict in the changelog where I should have merged it.

Well, the rebased commits are in master and they still show you as author (and me as committter), so it's not that bad ... I'll leave this as-is:

@cdeil cdeil closed this Aug 2, 2015

@cdeil cdeil referenced this pull request Aug 2, 2015

Merged

Improve Makefile #314

3 of 3 tasks complete

JonathanDHarris added a commit to JonathanDHarris/gammapy that referenced this pull request Sep 25, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment