@scheibelp: this looks good, though I think it should avoid modifying concrete specs even if they are not installed. Is there a reason that's needed?

Most of my checks didn't need this. But the normalize call in the concretization loop sets force=True, so I kept the check for package.installed in _mark_concrete.

@scheibelp Have you seen whether this interacts nicely with #4939? I can try to merge the two and test if you don't have time to.

@scheibelp Have you seen whether this interacts nicely with #4939? I can try to merge the two and test if you don't have time to.

@becker33 it shouldn't conflict but there may be some redundancy between the two: in particular this PR doing a check in _concretize_helper may be effectively the same as the checks you add to the beginning of each function in 4939 (EDIT) in concretize.py in #4939

I think it is sufficient and more correct in all of these cases to just check self.concrete. If the spec is concrete, we shouldn't change it, even if it's not installed. This should also be safe to do, since we only mark things concrete at the end of concretize now (since #5332 was merged)

@tgamblin I changed it everywhere except in Spec._mark_concrete

@scheibelp: looks good to me. I think it would be good if we could get rid of normalize(force=True) but that can wait.

1. Can you add a test for this? I am not sure of the best way to do it, but it seems like you could:

   1. first make a MockPackage that has no patch
   2. Install it
   3. Update it to require a patch
   4. Try to install another package that depends on the original install of the mock package

2. Will this properly cause a conflicts when used with incompatible dependencies? i.e., if I don't descend into a concrete dependency and try to merge constraints with it, how do I know it satisfies all the required constraints? Two scenarios:

   1. Suppose that A depends_on B, with a patch, but the installed B has no patch. Will concretization fail?
   2. Suppose that A depends_on B plus a variant, but the installed B doesn't have the variant. Will concretization fail?

Can you add a test for this?

I can do that - I'll aim for having that ready around 5pm.

That being said MockPackages cannot be installed. This is likely a good candidate for adding a definition to builtin.mock.

Will this properly cause a conflicts when used with incompatible dependencies? i.e., if I don't descend into a concrete dependency and try to merge constraints with it, how do I know it satisfies all the required constraints? Two scenarios:

This should still report conflicts for incompatible dependencies. It still descends everywhere and e.g. constrains deps. The only case where it doesn't descend is if the dependency to merge doesn't exist in spec_deps and the given spec is concrete. That only happens when the dependency is a build dependency that was stripped of the spec when it was read from the install directory.

This should still report conflicts for incompatible dependencies. It still descends everywhere and e.g. constrains deps. The only case where it doesn't descend is if the dependency to merge doesn't exist in spec_deps and the given spec is concrete. That only happens when the dependency is a build dependency that was stripped of the spec when it was read from the install directory.

Perfect. Can you ensure that the test checks the cases above? I think we should be good then.

I can do that - I'll aim for having that ready around 5pm.

I'm going to be later than I hoped but still hopefully have tests for this tonight

@tgamblin

So far I've got a test to make sure patches aren't added to installed specs at ee5e702

If I understand correctly you also want a test to check that if a new spec requests an installed dependency with a conflicting dependency that is still detected.

If I understand correctly you also want a test to check that if a new spec requests an installed dependency with a conflicting dependency that is still detected.

... with a conflicting constraint (version, variant, etc.).

If I understand correctly you also want a test to check that if a new spec requests an installed dependency with a conflicting dependency that is still detected.

... with a conflicting constraint (version, variant, etc.).

@tgamblin alright that is added in e3587b4