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

Allow installation of different repositories with the same name in a … #1366

Merged
merged 4 commits into from Dec 18, 2015

Conversation

mvdbeek
Copy link
Member

@mvdbeek mvdbeek commented Dec 18, 2015

…single install transaction.

Addresses issue #1338

@mvdbeek
Copy link
Member Author

mvdbeek commented Dec 18, 2015

Ping @gregvonkuster @bgruening

@gregvonkuster
Copy link
Contributor

@mvdbeek Thanks for looking into this issue. I think it would be better to not
restrict this enhancement to only repositories that contain tools.

Repositories in the tool shed are usually defined uniquely by a combination of repository name, owner and changeset revision. If I remember correctly, each repo_info_dict in this case has this structure:

{<repository name> :
(description, clone_url, changeset_revision, ctx_rev, owner, repositoy_dependencies, tool_dependencies)}

Have you found this to be correct? If so, the following may be a better approach for your check (this has not been tested!!):

Note that I've also added a break in the for loop so that is does not continue
looking if the owner is found.

If the above structure is not correct, then we just need the owner (and possibly the changeset_revision as well) rather than a tool_id. Will a combination of repository name and owner be sufficient to make this check?

owner = required_repo_infor_dict[required_repo_info_dict_key[4]
is_present = False
for repo_info_dict in all_repo_info_dicts:
for k, v in repo_info_dict.items():
if required_repo_info_dict_key == k:
if owner == v[4]
is_present = True
break
...

@mvdbeek
Copy link
Member Author

mvdbeek commented Dec 18, 2015

I don't think this is currently limited to tools, packages have a tool_id as well.
(Which indeed is a combination of name, owner, (version, if any is defined) and toolshed)
For example the testcase suite_white_ghosts does not specify any tool.

You're right about break, that saves an additional loop though.

@gregvonkuster
Copy link
Contributor

@mvdbeek Ok, sounds good! ;)

@mvdbeek
Copy link
Member Author

mvdbeek commented Dec 18, 2015

I'll add the break and do more testing, marking this WIP in the meantime.

@mvdbeek mvdbeek changed the title Allow installation of different repositories with the same name in a … WIP: Allow installation of different repositories with the same name in a … Dec 18, 2015
@mvdbeek
Copy link
Member Author

mvdbeek commented Dec 18, 2015

OK, i tested this now with the smaller white_ghost testsuite (which doesn't contain any tools) and also with the very large metavisitor suite that we will send out for peer-review soon.

@mvdbeek mvdbeek changed the title WIP: Allow installation of different repositories with the same name in a … Allow installation of different repositories with the same name in a … Dec 18, 2015
@bgruening
Copy link
Member

Works for me on with the test repo and does not break a few tools I tested.
@gregvonkuster what do you think?

@martenson
Copy link
Member

This looks great, thanks a bunch @mvdbeek !
Still testing but probably gonna merge soon.

@martenson
Copy link
Member

next beer is on me, @mvdbeek !

martenson added a commit that referenced this pull request Dec 18, 2015
Allow installation of different repositories with the same name in a …
@martenson martenson merged commit cced5c3 into galaxyproject:dev Dec 18, 2015
@gregvonkuster
Copy link
Contributor

@bgruening I think this is fine. ;)

@bgruening
Copy link
Member

Awesome! Thanks a bunch @mvdbeek!

@mvdbeek
Copy link
Member Author

mvdbeek commented Dec 18, 2015

Alright, thanks everyone!

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

Successfully merging this pull request may close these issues.

None yet

5 participants