Skip to content

using RawConfigParser instead of ConfigParser (closes #2541) #2542

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

Merged
merged 10 commits into from
Apr 25, 2018

Conversation

djarecka
Copy link
Collaborator

should fix #2541

@satra
Copy link
Member

satra commented Apr 13, 2018

could you please add a test verifying that the information returned from commit_hash from get_info() during testing matches the git-describe output.

@satra satra added this to the 1.0.3 milestone Apr 13, 2018
@djarecka
Copy link
Collaborator Author

@satra I've added, get_nipype_gitversion() always gives me an extra g. I removed it in the test, but can remove from __version__

@djarecka
Copy link
Collaborator Author

get_nipype_gitversion() doesn't work in docker, so circleCI fails. I'm testing in some containers I have here, and there is no gitpathgit

@satra
Copy link
Member

satra commented Apr 13, 2018

@djarecka - getting git version requires git installed in the container. there must be something that specifies pacakges to be installed into the container (not just the things being scattered).

@satra
Copy link
Member

satra commented Apr 13, 2018

@djarecka - sorry i was confusing projects. for nipype, we should update the container build to also have git installed.

@djarecka
Copy link
Collaborator Author

@satra ok, I wasn't sure for moment, git was matching both conversation ;-)

I simply added if get_nipype_gitversion() to the test, but we can also add git to our images (don't want to assume that everyone has git in a nipype container`

@djarecka
Copy link
Collaborator Author

@satra - installing git is not enough to get get_nipype_gitversion. It's not that the software has to be installed from git to get this gitpathgit?

and we go have git inside our containers:
https://github.com/nipy/nipype/blob/master/docker/generate_dockerfiles.sh#L75

@satra
Copy link
Member

satra commented Apr 13, 2018

if the software is installed inside from source the source directory is mounted and git path should work on the source directory.

@djarecka
Copy link
Collaborator Author

@satra in our dockers get_nipype_gitversion returns None

@satra
Copy link
Member

satra commented Apr 13, 2018

it only works in the source directory. is there not a git repo in the source directory? it's possible that we only copy the current state rather than clone the repo. for nipype it may be useful to have the entire repo available.

@djarecka
Copy link
Collaborator Author

I believe we copy here. Can we clone it somewhere else?

@satra
Copy link
Member

satra commented Apr 14, 2018

/src/nipype may already be a clone. so the test should be skipped if not in nipype's source directory, but at least in our tests on travis and circle, we should explicitly check for environment variables, change to the appropriate directory and run the test.

@djarecka
Copy link
Collaborator Author

Travis runs the test.

@djarecka
Copy link
Collaborator Author

@satra, @oesteban , @effigies - I'd appreciate some suggestions. I'm installing nipype from github, but I'm getting errors like error: unrecognized arguments: -n --cov, etc., so I'm missing some dependencies. Looks like I'm doing something wrong, but not sure how to fix it...

@effigies
Copy link
Member

You need pytest-xdist and pytest-cov, I think.

@djarecka
Copy link
Collaborator Author

yes, but I'm not sure why this is missing. In the original version we didn't install it in addition to nipype, pytest-xdist is in the requirements

@djarecka
Copy link
Collaborator Author

wait, I think I'm missing [all]

@djarecka
Copy link
Collaborator Author

@satra - ok, I did something that doesn't make sense, I took nipype from the main github repo, so I'm testing master, what is not very interesting... But at the end I'm not sure what you had in mind, should I somehow check the branch for a specific PR and take nipype from there, or you were thinking about something different? It's not clear what is the main goal. As I wrote before, the commit has is tested by travis

@satra
Copy link
Member

satra commented Apr 17, 2018

@djarecka - sth like this. travis/circle automatically pulls the source of the pull-request or branch.

@skipif(not on travis or circle):
def test_githash():
    # find source directory and make sure it's a git directory
    # find the git describe version
    # compare to the githash stored on install

@djarecka
Copy link
Collaborator Author

@satra yes, I know travis/circle automatically takes the source of the pull-request or branch, but I thought that I have to change the way we do it... I will just revert my changes to docker.

@effigies
Copy link
Member

What's the status of this one?

@djarecka
Copy link
Collaborator Author

sorry, i was just confused what else I should do in this PR. Not sure, what is the important difference between the test suggested by @satra and the test I already included.

@effigies
Copy link
Member

Your test looks fine to me.

  • get_nipype_gitversion returns None if not in a git repository, so you should skip correctly on CI.
    • It does import nipype rather than look for the git root of the test file, which has the potential for conflicts if there's an installed version and a dev copy being tested. I have that situation right now, so I'll pull your branch and check this.
  • get_nipype_gitversion runs git describe, so it hits the first two points in Satra's comments
  • get_info()['commit_hash'] checks the stored hash

@effigies
Copy link
Member

Yeah, this is passing locally both in my normal and a clean environment. I think this should be good.

@satra satra merged commit a1ab518 into nipy:master Apr 25, 2018
@djarecka
Copy link
Collaborator Author

I want to come back to this for moment. Trying to understand when get_nipype_gitversion can return commit hash. If I'm installing nipype with conda or pip (from github) this will never work, am I right?

@satra
Copy link
Member

satra commented Apr 29, 2018

@djarecka - only when the entire git source is present, i.e. whenever installed from a clone.

@djarecka
Copy link
Collaborator Author

@satra but there is any other way of checking git commit for package installed with conda or pip (without cloning)?

@satra
Copy link
Member

satra commented Apr 29, 2018

with conda/pip, one could potentially use the builder to insert COMMIT_INFO when building a conda package or a wheel package, but retrieving the info is a little tricky. on conda-forge the source hash of the repo is used (different from git hash).

the key problem is that the git hash changes as soon as you commit something to COMMIT_INFO. so one will always be one commit off.

for releases, we don't care about the git hash, so gitversion does not apply. gitversion was primarily for people who clone nipype and develop rather than users who install nipype.

@djarecka
Copy link
Collaborator Author

@satra - ok, thanks for explanation!

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.

InterpolationSyntaxError from get_info()
3 participants