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

Use conda-build=3 #5274

Merged
merged 22 commits into from
Apr 11, 2018
Merged

Use conda-build=3 #5274

merged 22 commits into from
Apr 11, 2018

Conversation

isuruf
Copy link
Member

@isuruf isuruf commented Feb 27, 2018

cc @conda-forge/core

  • Add c3i LICENSE
  • Restore the recipes/examples folder

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/example2, recipes/example3) and found it was in an excellent condition.

@isuruf
Copy link
Member Author

isuruf commented Feb 27, 2018

@msarahan, do you know why circleci works, but not appveyor?

@jakirkham
Copy link
Member

Because the example gets removed from PRs. Same story with AppVeyor. This would happen on CircleCI; however, as you are running CircleCI on your fork, the example is not removed. If you disabled CircleCI on your fork and pushed a fresh commit, it would have the same issue.

This comes down to this code or similar for the other CIs, which makes sure we don't build recipes in PRs that were merged into master. Expect if you commented that code or changed example2 and example3 to not rely on example, the problem would be resolved.

If you do merge these examples, would double check that this code keeps them from going into feedstocks. Seems like it should though. Might also be worth moving all the example recipes into one big example directory to keep the line noise low.

@isuruf
Copy link
Member Author

isuruf commented Feb 27, 2018

I've removed the example recipe for now. example2 now depends on example3, but example2 is built first. Is this a conda-build issue or was it never meant to replace conda-build-all?

@msarahan
Copy link
Member

conda-build can build more than one recipe at a time, but it currently does not do topological sorting for which should be built first. We'd like to add that eventually, but it's not high priority for us. We use conda-concourse-ci internally for this purpose.

@jakirkham
Copy link
Member

So I think the issue is example was moved to example3. Hence we are running into the same issue as before. Would restore example and just add a brand new copy of example3. Happy to push a commit to your PR if you would like assist.

@isuruf
Copy link
Member Author

isuruf commented Feb 27, 2018

conda-build can build more than one recipe at a time, but it currently does not do topological sorting for which should be built first.

Okay. I guess I can write a simple python script that uses conda_concourse_ci.compute_build_graph.construct_graph. @msarahan, I don't see conda-concourse-ci in defaults. Is it meant for internal use only?

@isuruf
Copy link
Member Author

isuruf commented Feb 27, 2018

So I think the issue is example was moved to example3.

I don't think that's an issue here, but feel free to push to this PR.

@isuruf
Copy link
Member Author

isuruf commented Feb 27, 2018

Here's a script that uses conda_concourse_ci. It uses code only from https://github.com/conda/conda-concourse-ci/blob/master/conda_concourse_ci/compute_build_graph.py. We can even vendor it, since it's just one file. We'll need a new dependency, networkx.

import conda_build.conda_interface
import networkx as nx
import conda_build.api
from conda_concourse_ci.compute_build_graph import construct_graph
import argparse
import os

def build_all(recipes_dir, arch, variant_config_files):
    index = conda_build.conda_interface.get_index(channel_urls=['conda-forge'])
    conda_resolve = conda_build.conda_interface.Resolve(index)

    config = conda_build.api.Config(variant_config_files=variant_config_files)

    worker={'platform': get_host_platform(), 'arch': arch, 'label': get_host_platform()}

    G = construct_graph(recipes_dir, worker=worker, run='build', conda_resolve=conda_resolve,
                        folders=os.listdir(recipes_dir), config=config)

    order = list(nx.topological_sort(G))
    order.reverse()

    print('Building packages')
    print('\n'.join(order))

    for node in order:
        conda_build.api.build(G.node[node]['meta'])


def get_host_platform():
    from sys import platform
    if platform == "linux" or platform == "linux2":
        return "linux"
    elif platform == "darwin":
        return "osx"
    elif platform == "win32":
        return "win"


if __name__ == "__main__":
    parser = argparse.ArgumentParser()
    parser.add_argument('recipes_dir', default=os.getcwd(), help='Directory where the recipes are')
    parser.add_argument('--arch', default='64', help='target architecture (64 or 32)')
    parser.add_argument('-m','--variant-config-files', action='append', help='variant config files')
    args = parser.parse_args()
    build_all(args.recipes_dir, args.arch, args.variant_config_files)

folders=os.listdir(recipes_dir), config=config)

order = list(nx.topological_sort(G))
order.reverse()
Copy link
Member

Choose a reason for hiding this comment

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

Why is the build order reversed? (wouldn't this build the bottom of the graph first?)

Copy link
Member

Choose a reason for hiding this comment

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

Could be that I build the graph backward when adding the nodes. I have considered going back and reversing it, but haven't found time. When I have tried, it ended up exploding in my face.

Copy link
Member Author

Choose a reason for hiding this comment

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

It depends on how the edge direction is defined. reverse here gives the order that I need

@@ -10,7 +10,7 @@
# `conda install openssl -c conda-forge``

package:
name: {{ name|lower }}
name: simplejson2
Copy link
Member

Choose a reason for hiding this comment

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

Is this on purpose?

Copy link
Member Author

@isuruf isuruf Feb 27, 2018

Choose a reason for hiding this comment

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

Yes, but I'll be reverting the examples once I know this works.

@isuruf
Copy link
Member Author

isuruf commented Feb 27, 2018

This passes on appveyor in my fork.
I can use some help in getting linux to work. Need to copy the python files in .ci_support to the docker container and use them.

@isuruf
Copy link
Member Author

isuruf commented Feb 28, 2018

Strange errors in linux and osx. @msarahan, any suggestions?

@msarahan
Copy link
Member

In both cases, it's something to do with the system compilers and the resulting binary search path:

https://travis-ci.org/conda-forge/staged-recipes/builds/346956154#L302

On circle:

Fixing permissions
patchelf: file: /home/conda/staged-recipes/build_artifacts/simplejson2_1519761708098/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold/lib/python2.7/site-packages/simplejson/_speedups.so
    setting rpath to: $ORIGIN/../../..
WARNING (simplejson2,_speedups.so): did not find - or even know where to look for: /lib64/libc.so.6
WARNING (simplejson2,_speedups.so): did not find - or even know where to look for: /lib64/libpthread.so.0

I don't know anything more off the top of my head. I'll try to reproduce in a docker container tomorrow. @mingwandroid might have more ideas than me.

@isuruf
Copy link
Member Author

isuruf commented Feb 28, 2018

I can reproduce this locally on linux.

It tries to test a py2.7 package on a py3.6 environment. definitely something wrong with my script


The following NEW packages will be INSTALLED:

    ca-certificates: 2018.1.18-0  conda-forge                                      
    ncurses:         5.9-10       conda-forge                                      
    openssl:         1.0.2n-0     conda-forge                                      
    python:          3.6.4-0      conda-forge                                      
    readline:        7.0-0        conda-forge                                      
    simplejson2:     3.8.2-py27_0 file:///home/conda/staged-recipes/build_artifacts
    sqlite:          3.20.1-2     conda-forge                                      
    tk:              8.6.7-0      conda-forge                                      
    xz:              5.2.3-0      conda-forge                                      
    zlib:            1.2.11-0     conda-forge 

@isuruf
Copy link
Member Author

isuruf commented Feb 28, 2018

Tests pass. Don't merge till I restore the example folder.

Can I get a review?

@msarahan
Copy link
Member

What conda version are you using? You should be aware of conda/conda-build#2707. This sounds potentially related.

TLDR: probably better to pin conda to 4.3.x for now. Kale is working on a fix.

.appveyor.yml Outdated
@@ -5,13 +5,12 @@ environment:

# Workaround for https://github.com/conda/conda-build/issues/636
PYTHONIOENCODING: "UTF-8"
CONDA_INSTALL_LOCN: "C:\\Miniconda36-x64"
Copy link
Member

Choose a reason for hiding this comment

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

better to leave this as Miniconda3-x64 - that's a symlink to the current miniconda3 install on appveyor. It'll be better to keep up-to-date that way (fewer updates, potentially)

@@ -0,0 +1,597 @@
#!/usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty big to vendor. It's probably OK, but it would be good to re-examine this vendoring decision in a few months and see if it's still a good idea, or if it would be better to add a dependency on c3i.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer adding c3i as a dependency, but c3i is not on defaults, so I thought it was meant for internal use only.

Copy link
Member

Choose a reason for hiding this comment

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

There's also a lot of unused stuff here. I'm not sure if you want to remove it, or just leave it in for easier diffs down the line, should there be changes to both c3i and this file.

Copy link
Member

Choose a reason for hiding this comment

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

We can work on getting it onto defaults. It has only seen internal use, but I wouldn't say it's only meant for internal use. It's just much more oriented towards our needs, but could grow more general if there's community use and input (PRs definitely welcome).

Copy link
Member

Choose a reason for hiding this comment

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

Sorry to be late. What was the outcome on this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

This functionality will be split from c3i and make its way into conda-build. No timeline right now, but hopefully soon. I don't think it will take too much work to have it simply work. Having it go fast will take more time.

@isuruf
Copy link
Member Author

isuruf commented Apr 9, 2018

Is the concern that recipes requiring conda-build 3 get merged before we release conda-smithy 3?

Yes.

How would you recommend we address this? Documentation? Something else?

Release conda-smithy=3 first since anything working on cb2 works with cb3, there will be no issues.

Merge this PR after conda-smithy=3 is released, then recipes with cb3 features will work in both staged-recipes and feedstocks

@jakirkham
Copy link
Member

Merge this PR after conda-smithy=3 is released, then recipes with cb3 features will work in both staged-recipes and feedstocks

So this gets at my larger question. Are we generally ok with merging this as is (aside from merge conflicts and order of merges)?

@scopatz
Copy link
Member

scopatz commented Apr 10, 2018

@jakirkham - yes, I think everyone I talked to yesterday at the con about his @ocefpaf @CJ-Wright @ericdill @jjhelmus etc are OK with it

@mwcraig
Copy link
Contributor

mwcraig commented Apr 10, 2018

Not there, but I'm in favor of this moving ahead....

@scopatz
Copy link
Member

scopatz commented Apr 10, 2018

Hmm I resolved the conflicts but it looks like it failed...

@msarahan
Copy link
Member

fail seems like a conda update related thing. Kale is no longer tolerating channels that don't have noarch. I will hack in a fix in a bit.

.appveyor.yml Outdated
@@ -3,7 +3,7 @@ skip_commits:

environment:

CONDA_INSTALL_LOCN: "C:\\Miniconda36-x64"
CONDA_INSTALL_LOCN: "C:\\Miniconda3-x64"
Copy link
Member Author

Choose a reason for hiding this comment

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

Please don't. conda/conda#7144

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... OK. I was probably chasing a red herring with the conda exe thing. I will revert this. Thanks for posting the issue.

Copy link
Member

Choose a reason for hiding this comment

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

Given recent issues this seems to solve, are we ok leaving Miniconda36 here short term? This may already be agreeable at this stage. Just trying to confirm.

@scopatz
Copy link
Member

scopatz commented Apr 11, 2018

Pulling the trigger.

@scopatz scopatz merged commit b12f7e7 into conda-forge:master Apr 11, 2018
@ocefpaf
Copy link
Member

ocefpaf commented Apr 11, 2018 via email

@scopatz
Copy link
Member

scopatz commented Apr 11, 2018

I agree @ocefpaf :)

@isuruf
Copy link
Member Author

isuruf commented Apr 11, 2018

Milestone for conda-smithy=3 is not yet complete, https://github.com/conda-forge/conda-smithy/milestone/7

@isuruf isuruf deleted the cb3 branch April 11, 2018 16:21
@ocefpaf
Copy link
Member

ocefpaf commented Apr 11, 2018 via email

@isuruf
Copy link
Member Author

isuruf commented Apr 11, 2018

Please test before releasing. I'd urge everyone from core to rerender at least 2 of their favourite recipes and see if it works on CI

@ocefpaf
Copy link
Member

ocefpaf commented Apr 11, 2018

On it. Thanks @isuruf!

@scopatz
Copy link
Member

scopatz commented Apr 11, 2018

I have tried rerendering a few of the harder packages, such as cyclus, and everything works like a dream

@isuruf
Copy link
Member Author

isuruf commented Apr 11, 2018

Thanks @scopatz. Did you also try on CI?

@scopatz
Copy link
Member

scopatz commented Apr 11, 2018

Not yet.

@isuruf
Copy link
Member Author

isuruf commented Apr 11, 2018

Please have a look at https://github.com/conda-forge/conda-smithy/wiki/Release-Notes-3.0.0.rc1 and update if you find any mistakes

@jakirkham
Copy link
Member

jakirkham commented Apr 12, 2018

Had been playing with setuptools re-rendering yesterday and today (note that compiles code on Windows specifically). Seems to work fine after ironing out a few bugs earlier today.

xref: conda-forge/setuptools-feedstock#82

@scopatz
Copy link
Member

scopatz commented Apr 12, 2018

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

Successfully merging this pull request may close these issues.

9 participants