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

conda 4.4 compatibility #96

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

kalefranz
Copy link
Contributor

The only change here for conda 4.4 is a small tweak in logging initialization. I just got my first issue reported that I believe to be a result of thrashing between conda 4.3 and 4.4: conda/conda#6804. Conda 4.4 has been out for over a month now, and it's been pretty stable since the initial release (was in canary for months). Probably time to get the conda-forge upgrade pushed through for conda-forge users.

@kalefranz
Copy link
Contributor Author

So the failing test here caused me to find a problem in conda 4.4.8. Have a PR to fix it now.

Copy link
Contributor

@pelson pelson left a comment

Choose a reason for hiding this comment

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

Thanks for this. Looks very sensible.

Test failed with conda 4.4.8-py35_0. Any chance of 4.4.9 being released soon @kalefranz?

@kalefranz
Copy link
Contributor Author

Gosh I thought i had this fixed on conda 4.4.9. Guess it's going to take another go.

@mbargull
Copy link

conda.resolve.Resolve.version_key expects conda.resolve.Resolve.index[key] to have a .channel attribute. But conda.resolve.Resolve.index.values() are never converted from dict instances.
Here is a pseudo-Python backtrace of the used index:

r.version_key(dist):
    info_dict = r.index[dist]
    channel = info_dict.channel  # BOOM
r = Resolve(index)
index = {conda.models.dist.Dist(...): info_dict, ... }  # index = copy_index(index) = {conda.models.dist.Dist(key): index[key] for key in index}
index = {"name-version-build_id": info_dict, ...}  # index = {distro.pkg_fn(): distro.info_index() for distro in distros}
distros = [conda_build.Metadata(...), ...]

So, in copy_index only the keys instantiate Dists, the values remain the same dicts as returned from conda_build.Metadata.info_index(). Resolve.version_key in conda 4.3.x only used .get on those values but never directly accessed an attribute like .channel.

@kalefranz
Copy link
Contributor Author

So the quick, but brittle, fix here is to just use the get() method instead of attribute access I guess?

@kalefranz
Copy link
Contributor Author

BTW, for future reference, we do plan on eliminating dist. The history here is that dist is derived just from the name of the tarball (or maybe the full URL of the tarball). That's not what we want to be using as the source of truth for package metadata information. We're all set up to switch out Dist for PackageRef in almost all situations. So that's coming... Someday...

@mbargull
Copy link

I'm a little confused why the keys are conda.models.dist.Dist(key) with key = distro.pkg_fn() and thus isinstance(key, str) -- did I miss something else?

@mbargull
Copy link

Also, I haven't looked at what happened with conda._vendor.auxlib.exceptions.ValidationError: Invalid value 0 for build_number.

@kalefranz
Copy link
Contributor Author

I don't know why the build number thing just popped up all the sudden. I'm guessing it's complaining though because build_number needs to be an integer and not a string.

@mbargull
Copy link

Ah, your last comment didn't load for me while posting.

@mbargull
Copy link

It looked as though build_number comes from conda_build.Metadata.build_number() which returns an int AFAICT -- have to take a closer look though.

@mbargull
Copy link

@mbargull
Copy link

So, if a "0" was indeed a valid input at some point, you'd have to take care of that conversion while calling https://github.com/conda/conda/pull/6808/files#diff-39ec17febc24e90aa82fc8390d50ec87R207, I suppose? If you are very certain that it was never a valid input and didn't exist in real data, you could of course just adjust the DummyIndex to use ints instead of strings..

@kalefranz
Copy link
Contributor Author

build_number has been enforced as an integer for quite some time. https://github.com/conda/conda/blob/4.3.0/conda/models/index_record.py

It feels like something recently changed with the test data in this repo. I’ll investigate.

@mbargull
Copy link

But in conda 4.3.x no IndexRecord is instantiated when a MatchSpec is created, I think. So for conda-build-all, which directly interacts with the conda.resolve.Resolve object, there might have previously be no IndexRecord instantiation at all?

@kalefranz
Copy link
Contributor Author

Yeah I agree that’s what it’s looking like

@kalefranz
Copy link
Contributor Author

Just pushed a change that makes that test build number an int. I think this is safe. We've been enforcing integer build numbers for over a year, and I've never seen a problem there.

@kalefranz
Copy link
Contributor Author

Guess there's something going on with 4.4 and special_case_version_matrix(). Implementation details of MatchSpec have changed in 4.4, and it looks like that code is digging into some of the internals. So need to dig in there now and find a solution.

@kalefranz
Copy link
Contributor Author

I've isolated the remaining test failure here to be coming from conda-build 3.x. When I locally install conda-build 2.1.x, the test passes.

@kalefranz
Copy link
Contributor Author

kalefranz commented Mar 3, 2018

🎉 Tests are finally passing.

.travis.yml Outdated
@@ -38,7 +38,7 @@ install:

# Now do the things we need to do to install it.
- conda install -c conda-forge --file requirements.txt nose mock python=${PYTHON} ${EXTRA_DEPS} --yes --quiet
- if [[ -n ${CONDA_ORIGIN} ]]; then conda install -yq -c ${CONDA_ORIGIN} conda conda-build; fi
- if [[ -n ${CONDA_ORIGIN} ]]; then conda install -yq -c ${CONDA_ORIGIN} conda conda-build=2.1; fi
Copy link
Member

Choose a reason for hiding this comment

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

Hmm...did we loose cb3 compatibility? Admittedly some things in cb3 weren't supported yet ( #97 ), but I thought it did work at least ( #93 ). 😕

@jakirkham
Copy link
Member

FWIW there has been some work to drop conda-build-all from conda-forge in PR ( conda-forge/staged-recipes#5274 ). ATM am not sure whether that is the best way to go or pushing conda-build-all is. @isuruf is probably in the best place to answer this question. That said, I think we should come to some conclusion on that and this may effect how you decided to prioritize this work @kalefranz. Admittedly people do use conda-build-all outside of conda-forge, so this work is likely to be appreciate regardless of what direction conda-forge picks.

@kalefranz
Copy link
Contributor Author

I actually thought there wasn’t compatibility yet with conda-build 3. So maybe there’s just one interaction between conda 4.4, conda-build 3, and conda-build-all. Test failure is at #97

@pelson
Copy link
Contributor

pelson commented Apr 11, 2018

I'm completely happy to merge the code-changes, but don't really want to loose the cb3 testing if possible. Is there anything I can do to help get your changes in combination with the original cb3 test?

@@ -5,7 +5,41 @@

CONDA_VERSION_MAJOR_MINOR = tuple(int(x) for x in CONDA_VERSION.split('.')[:2])

if (4, 3) <= CONDA_VERSION_MAJOR_MINOR < (4, 4):
if (4, 4) <= CONDA_VERSION_MAJOR_MINOR < (4, 5):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this equivalent to: if CONDA_VERSION_MAJOR_MINOR == (4,4)?

@pelson
Copy link
Contributor

pelson commented May 23, 2018

Ping @kalefranz. I think this is pretty much good to go, I'm just looking to avoid removing the cb3 testing if possible.

@kalefranz
Copy link
Contributor Author

There was one test failure that pinning to conda-build 2.1 fixed.

https://travis-ci.org/conda-tools/conda-build-all/jobs/341129488#L803

So I guess we have some interaction between the new matchspec in conda, new conda-build, and conda-build-all here. I so far haven't been able to track down what it is.

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.

5 participants