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

Putting asv.conf.json on project top level #308

Merged
merged 1 commit into from
Dec 1, 2015

Conversation

pv
Copy link
Collaborator

@pv pv commented Aug 22, 2015

Putting asv.conf.json on project repository top level has the advantage that running asv commands no longer requires cd to somewhere else. However, it usually also requires putting the repository clone to somewhere else than conf.project.

  • Add config option mirror_dir for specifying the mirror location. Change the default value be "project" --- the mirror is a bare repository, so looking inside it is not usually useful.
  • Make asv quickstart provide also a default config more suitable for putting on project top level.

This also requires gh-307 to work

@@ -181,11 +181,11 @@ def get_repo(conf):
if conf.dvcs is not None:
for cls in util.iter_subclasses(Repo):
if getattr(cls, 'dvcs') == conf.dvcs:
return cls(conf.repo, conf.project)
return cls(conf.repo, conf.mirror_dir)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like an API change -- I think if conf.mirror_dir is not provided, it should fall back to the value of conf.project, whereas it appears that now it will default to (the literal) "project".

Copy link
Collaborator Author

@pv pv Aug 24, 2015 via email

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

It probably doesn't matter, but it seems relatively cheap to maintain the old behavior here so we probably should.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, changed the default value.

@pv pv force-pushed the samerepo branch 2 times, most recently from e7629af to 79cd793 Compare August 24, 2015 17:10
@pv
Copy link
Collaborator Author

pv commented Aug 24, 2015

One alternative to mirror_dir could be to not use a mirror repository for local repositories.
This could be a cleaner solution to the problem: master...pv:no-mirror-for-local

@mdboom
Copy link
Collaborator

mdboom commented Aug 24, 2015

In the alternative solution, the project url becomes a local filesystem path, right. Doesn't that then mean the responsibility is on the user (or something outside of asv) to make sure that repo is updated? As it stands, asv handles pulling from a remote repo.

@pv
Copy link
Collaborator Author

pv commented Aug 24, 2015

It's the user's responsibility to update the source repo always when using a local path.
asv doesn't do anything to the source repo except cloning from it, all operations are done on the mirror one (and pull() on the mirror repo just pulls from the source repo on the local filesystem).

@mdboom
Copy link
Collaborator

mdboom commented Aug 24, 2015

Sorry -- that's right: I haven't used local paths much myself, but that seems like the right thing to do. (i.e. asv manages it entirely or the user manages it and asv treats it as "read-only").

@pv
Copy link
Collaborator Author

pv commented Aug 24, 2015

Yes, the effect in the end is the same as if there was no mirror repository when the path is local. The only difference AFAICS is that you can't use the HEAD and FETCH_HEAD references and the reflog syntax master@{3} since these are not preserved by clone --mirror. Since these are often useful, it would be best to just skip the mirroring.

@pv
Copy link
Collaborator Author

pv commented Aug 25, 2015

Actually, it probably doesn't hurt to have both mirror_dir config variable and to not use mirrors for local repos. The other directory names are also configurable, and the project config also affects what's shown in the HTML output.

@pv
Copy link
Collaborator Author

pv commented Sep 28, 2015

Dropped the mirror_dir stuff, but left the quickstart changes...

@pv
Copy link
Collaborator Author

pv commented Oct 17, 2015

Planning to merge soon.

@mdboom
Copy link
Collaborator

mdboom commented Oct 18, 2015

I think the Python 2.6 failure on AppVeyor is just a spurious something... I've restarted.

@pv
Copy link
Collaborator Author

pv commented Oct 18, 2015

The appveyor builds have sporadic failures with "error 5: permission denied" on rmtree-ing build environments. We have the one knownfailure test that exhibits this reliably on some (but not all) python versions, but apparently similar issue can appear elsewhere.

It's unclear how specific this is to appveyor; I don't recall being able to repro them locally, and don't have a VM at hand currently. gh-280 has some discussion.

mdboom added a commit that referenced this pull request Dec 1, 2015
Putting asv.conf.json on project top level
@mdboom mdboom merged commit 7a955c2 into airspeed-velocity:master Dec 1, 2015
@coveralls
Copy link

coveralls commented May 26, 2017

Coverage Status

Coverage increased (+0.2%) to 53.427% when pulling 79cd793 on pv:samerepo into d82dcde on spacetelescope:master.

@pv pv deleted the samerepo branch August 19, 2018 20:07
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.

None yet

3 participants