conda-tools / conda-build-all

BSD 3-Clause "New" or "Revised" License
30 stars 24 forks source link

[travis] add conda-4.3 to build matrix and fix test cases #83

Closed marscher closed 7 years ago

marscher commented 7 years ago

purpose is to test upcoming changes in conda early. eg. #76

jakirkham commented 7 years ago

Thanks for doing this @marscher. 😄

marscher commented 7 years ago

I hope my workarounds are not too hacky (some of them can be removed in the future, because wrapping in Dist objects will be done automatically by conda, eg. https://github.com/conda/conda/pull/4931)

marscher commented 7 years ago

I'd be glad, if somebody more familiar with the package could review this :)

jakirkham commented 7 years ago

If you have time, would really appreciate a review here @marqh. The goal here is to add support for conda 4.3.x.

jakirkham commented 7 years ago

Would add that PR ( https://github.com/conda/conda/pull/4931 ) was closed. So I don't think this fix will land in conda after all. As such this will be the permanent fix until conda-build-all starts handling Dist objects directly.

jakirkham commented 7 years ago

Friendly nudge @marqh. 😉

marqh commented 7 years ago

hello @jakirkham @marscher

I am concerned that my understanding of this pull request is pretty low. The tests pass, which provides some confidence, but the subtleties of potential behaviour change are not evident to me.

I don't think this should stand in the way of this, but I think some developer comments in the code detailing the decision paths and dependencies would help. I am not sure what I would write though, so if this seems unnecessary, please let me know.

I assume that this is desired within a release, is this correct? Does this represent a behaviour change, and hence point release (1.1) rather than a bug fix (1.0.4)?

jakirkham commented 7 years ago

My gut feeling is this is a patch release. It still attempts to handle the new conda behavior. Though if it runs into issues, it falls back to the old conda behavior. No new API is included in this change and no changes are added to the existing API.

jakirkham commented 7 years ago

Any thoughts on @marqh's comments @marscher?

marscher commented 7 years ago

I would also prefer if some of the conda devs would have a tiny look at this. So far I've been careful not to include any new stuff, which break backward compatibility. Is conda-build-all pinned to some version in conda-forge so we can carefully test this prior unpinning/releasing?

jakirkham commented 7 years ago

Have asked @kalefranz to take a look at this when he has a chance.

msarahan commented 7 years ago

This seems sane to me. I'd say merge it.

jakirkham commented 7 years ago

Thanks for looking @msarahan. 😄

kalefranz commented 7 years ago

LGTM also. We're trying to get rid of Dist altogether. But I don't think that's going to happen in 4.4. So this PR is probably safe for both 4.3 and 4.4, at least regarding index access.

kalefranz commented 7 years ago

@stefanseefeld is going to help push conda 4.3 integration with conda-build-all and other conda-forge tools across the finish line.

jakirkham commented 7 years ago

@marqh @marscher, could either (or both) of you please comment on what you think is still necessary to complete for this to proceed?

marscher commented 7 years ago

Since there has been some review from upstream, I think this is in a mergeable state.

jakirkham commented 7 years ago

Any chance we can get this merged and released @marqh?

jjhelmus commented 7 years ago

Would also like to see this merged soon so conda-forge can move to using conda 4.3. Let me know if there is anything I can do to help move this process forward.

kalefranz commented 7 years ago

FWIW, I consider conda 4.2 past its end of life. It hasn't been updated in quite a while. The conda-forge maintainers are doing a real disservice to the conda-forge user community for keeping them on conda 4.2.

jakirkham commented 7 years ago

Unfortunately there are no active conda-forge maintainers with permissions to this repo, @kalefranz. What would you propose?

kalefranz commented 7 years ago

Isn't that a problem then? Since the whole conda-forge toolchain is being held hostage here? Why don't you fork this repo under the conda-forge organization and build from there?

kalefranz commented 7 years ago

We could also just make a patch from this pr, apply it to the feedstock, and bump the build number.

On May 25, 2017, at 9:51 AM, jakirkham notifications@github.com wrote:

Unfortunately there are no active conda-forge maintainers with permissions to this repo, @kalefranz. What would you propose?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

marscher commented 7 years ago

Thats actually a good solution for the meantime.

jjhelmus commented 7 years ago

The conda-forge organization has forked this repository, https://github.com/conda-forge/conda-build-all. I've submitted this PR against that fork, conda-forge/conda-build-all#1