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

Check for namespace-qualified packages in repo_for_pkg #5787

Merged

Conversation

scheibelp
Copy link
Member

Fixes #5754

Previously when RepoPath.repo_for_pkg was invoked with a string,
it did not check if the string included a namespace. Any
namespace-qualified package provided as a string would not be found
(at which point the behavior was to return the highest-precedence
repository).

Previously when RepoPath.repo_for_pkg was invoked with a string,
it did not check if the string included a namespace. Any
namespace-qualified package provided as a string would not be found
(at which point the behavior was to return the highest-precedence
repository).
@scheibelp
Copy link
Member Author

Recently, #5716 added calls to Spec.package_class which invokes RepoPath.repo_for_pkg with a namespace-qualified name. Other instances of Spec.package_class would have broken before but adding the calls in #5716 made it "more likely" to encounter an issue when there were multiple repositories.

@scheibelp
Copy link
Member Author

scheibelp commented Oct 16, 2017

I'll be able to add a unit test for this later today.

(EDIT: didn't hit original target for 10/16 so I'll aim for tomorrow, 10/17)

@tgamblin tgamblin self-requested a review October 17, 2017 03:45
Copy link
Member

@tgamblin tgamblin left a comment

Choose a reason for hiding this comment

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

One minor change request. I also think we should definitely get some tests in for namespaced packages.

else:
# handle strings directly for speed instead of @_autospec'ing
name = spec
try:
namespace, name = spec.split('.')
Copy link
Member

@tgamblin tgamblin Oct 17, 2017

Choose a reason for hiding this comment

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

Namespaces can be nested, so I think you want:

namespace, _, name = spec.rpartition('.')

to do this right. This is what Repo uses. namespace will be empty if there is no namespace.

@scheibelp
Copy link
Member Author

@tgamblin added tests and made suggested update.

To make this more-easily tested (in particular separable from global state), ef360f5 replaced the spec.virtual call in Repo.get

@scheibelp
Copy link
Member Author

@yee379 as far as I can tell I can't reply directly to your comment at 65b3876#commitcomment-25042386 but this PR should fix your issue (which is likely another case of #5754).

# given RepoPath
@pytest.fixture()
def test_repo():
return spack.repository.RepoPath(spack.mock_packages_path)
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, pytest looks for methods and functions that start with test_ and consider them as tests. Not sure if creating a fixture that matches this pattern is asking for troubles...

@scheibelp
Copy link
Member Author

Hmmm, pytest looks for methods and functions that start with test_ and consider them as tests. Not sure if creating a fixture that matches this pattern is asking for troubles...

I have edited the name - thanks for pointing that out!

@tgamblin tgamblin merged commit dcdd16b into spack:develop Oct 25, 2017
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