Updates for dual compiler output mode under travis and circle #836

mariusvniekerk · 2018-07-15T16:19:38Z

This ties with conda-forge/conda-forge-pinning-feedstock#92

isuruf · 2018-07-15T16:23:38Z

conda_smithy/templates/upload_package.py.tmpl

@@ -0,0 +1,27 @@
+# This file was generated automatically from conda-smithy. To update this configuration,


This should be done in the upload script itself. There's no need to have 2 scripts where one calls the other.

The issue is mostly that we need to have the jinja templates around

You mean the ${CONFIG} env variable?

isuruf · 2018-07-15T16:24:35Z

conda_smithy/templates/update_condarc.py.tmpl

@@ -0,0 +1,30 @@
+# This file was generated automatically from conda-smithy. To update this configuration,
+# update the conda-forge.yml and/or the recipe/meta.yaml.


This file can be moved to conda-forge-ci-setup as well.

mariusvniekerk · 2018-07-15T17:45:24Z

conda_smithy/templates/upload_package.py.tmpl

+import sys
+
+global_config = json.loads('''
+{{ channels | tojson }}''')


This is the jinja render that that we include here. This provides the fallback for recipes that do not have the channel_targets / channel_sources in their conda_build_config files.

Can we read .conda-forge.yml and get this value?

We don't presently persist this anywhere. I guess we could

scopatz · 2018-07-17T14:25:41Z

LGTM

Simplified utility script

mariusvniekerk · 2018-07-18T01:17:27Z

Okay ready for another review

isuruf · 2018-07-18T01:29:03Z

conda_smithy/templates/build_utils.py.tmpl

+def make_build_number(args):
+    specific_config = safe_load(open(args.config_file))
+    build_number_inc = specific_config.get("build_number_increment", [0])[0]
+    rendered_recipe = subprocess.check_output(['conda', 'render', '-m', args.config_file, args.recipe_root])


What happens when you have multiple outputs?

Would we expect them to all share a build number? Or can that vary per output?

isuruf · 2018-07-18T01:30:06Z

conda_smithy/templates/build_utils.py.tmpl

+    try:
+        build_number = int(rendered['build']['number'])
+    except KeyError:
+        build_number = 0


In this case, clobbered file should be removed

mariusvniekerk · 2018-07-18T02:29:14Z

Reworked the build number bump to deal with multiple outputs.
Presently conda-build does not have a way to clobber/append a value in the outputs section, thus we are forced to revert to basic string replacement 😢

mariusvniekerk · 2018-07-18T03:02:41Z

conda_smithy/templates/build_utils.py.tmpl

+
+def mangle_compiler(args):
+    """Try hard to break the compilers for osx"""
+    # TODO


What are some good suggestions for things to do here to break compilers?

What do you mean by this?

Basically something we can do to make osx NOT have compilers that are found. Essentially this is so that we don't accidentally build with system compilers when we want to build with the Anaconda ones.

This is easy on linux since its just a docker switch

isuruf · 2018-07-18T23:09:24Z

Reworked the build number bump to deal with multiple outputs. Presently conda-build does not have a way to clobber/append a value in the outputs section, thus we are forced to revert to basic string replacement

I don't think there are any recipes with different build numbers for different outputs. Therefore clobber file is fine, but we should make rendering of the output of conda render recipe is working for multiple outputs. Sorry for being unclear before

mariusvniekerk · 2018-07-19T02:37:09Z

I did some tests and there is nothing intrinsically preventing you from having different build numbers per output. The alternative approach would be to use the conda_build.api.render method

jakirkham · 2018-07-19T02:40:18Z

vtk uses different build numbers for different variants of packages. Not sure how MPI packages are dealt with these days.

mariusvniekerk · 2018-07-19T02:52:07Z

we also use them as markers for things like blas=*=openblas. Presumably we chan check if they are int-like?

isuruf · 2018-07-19T03:00:35Z

Those have different build numbers, but one build number per variant. Thats' fine. What I'm asking is, is there a use case where one variant (one .yaml in .ci_support) have multiple outputs with different build numbers?

jakirkham · 2018-07-19T03:17:54Z

Mentioned those because clobbering the build number in those cases will require some finesse.

Don’t think there are that many split packages. Also can’t see a good reason for them to deviate in this way (assuming it is possible)

isuruf · 2018-07-19T03:21:35Z

A clobber file is better because you are dealing with a rendered recipe. Regexes like what we do in the last commit is error prone.

jakirkham · 2018-07-19T03:29:08Z

However it doesn’t seem to let us add to the build number. It simply replaces it, which doesn’t seem flexible enough. Though if someone can prove me wrong, that would be good.

jakirkham · 2018-07-19T03:31:09Z

Should add it is worth keeping in mind this code is transient. We want it to work, but perfecting is probably not worthwhile in the long run.

isuruf · 2018-07-19T03:38:58Z

However it doesn’t seem to let us add to the build number. It simply replaces it, which doesn’t seem flexible enough. Though if someone can prove me wrong, that would be good.

What are you suggesting here? Are you saying regex replacement is better than clobber file which changes the build number for each variant?

mariusvniekerk · 2018-07-19T03:42:41Z

clobber files do not work for this case.

build: 
   number: 8

outputs:
  - name: foo
    build:
        number: 10
  - name: foo2

When rendered with a clobber file this makes the foo2 have the clobbered build number and foo not.

isuruf · 2018-08-01T22:08:05Z

conda_smithy/templates/build_utils.py.tmpl

+import argparse
+
+global_config = json.loads('''
+{{ channels | tojson }}''')


Can this be read from the conda-forge.yml file instead of a jinja template?

After that we don't need to vendor this file in every feedstock

yep, that could be done. Right now we only use that for storing tokens right?

isuruf · 2018-08-01T22:09:26Z

conda_smithy/templates/build_utils.py.tmpl

+
+        if build_number_int < 1000:
+            if not use_legacy_compilers:
+                raise ValueError("Only legacy compilers only valid with build numbers < 1000")


Instead of erroring, can we remove new compiler configuration?

This will catch the situation where we want to turn on new compilers but forgot to bump the build_number.

scopatz · 2018-08-06T16:27:34Z

Is this ready to go in now?

mariusvniekerk · 2018-08-06T17:11:53Z

Nope not yet

This environment variable is used for rendering variants with the dual compiler stack.

mariusvniekerk · 2018-08-13T18:12:42Z

This should be ready to go in now. Let me know if i need to squash-rebase some things down

scopatz · 2018-08-13T19:19:38Z

@mariusvniekerk - I am not sure I understand where setup_conda_rc comes from.

… pinnings

mariusvniekerk · 2018-08-13T20:13:32Z

@scopatz setup_conda_rc is provided by conda-forge-ci-setup=2.*

scopatz · 2018-08-13T20:45:10Z

Oh I see. This looks good then. It simplifies the logic here a lot. I'll merge tomorrow if no-one beats me to it.

scopatz · 2018-08-14T12:58:01Z

Thanks @mariusvniekerk!

jakirkham · 2018-08-22T16:53:01Z

conda_smithy/configure_feedstock.py

+            try:
+                config['channel_targets'] = [config['channel_targets'][0]]
+            except KeyError:
+                config['channel_targets'] = ['conda-forge main']


Should this have a / or something between these two?

jakirkham · 2018-08-22T16:54:04Z

conda_smithy/configure_feedstock.py

+            try:
+                config['channel_sources'] = [config['channel_sources'][0]]
+            except KeyError:
+                config['channel_sources'] = ['conda-forge,defaults']


It looks like this is a comma separated list within a YAML list. Should this also be a YAML list?

jakirkham · 2018-08-22T16:54:47Z

Sorry for the very late review comments. Noticed a few really minor things during re-rendering, which are noted above.

jakirkham · 2018-08-22T19:05:17Z

conda_smithy/templates/run_docker_build.sh.tmpl

@@ -29,6 +29,9 @@ if [ -z "$CONFIG" ]; then
    exit 1
 fi

+pip install shyaml


Would it make sense to add this to conda-forge?

xref: conda-forge/staged-recipes#6557

Updates for dual compiler output mode under travis and circle

d24570a

isuruf reviewed Jul 15, 2018

View reviewed changes

mariusvniekerk mentioned this pull request Jul 15, 2018

WIP: DO NOT MERGE conda-forge/zlib-feedstock#23

Closed

isuruf reviewed Jul 15, 2018

View reviewed changes

mariusvniekerk commented Jul 15, 2018

View reviewed changes

mariusvniekerk mentioned this pull request Jul 15, 2018

Dual compilers on osx and linux conda-forge/conda-forge-pinning-feedstock#92

Merged

Updates for dual compiler output mode under travis and circle

c4d7f1f

mariusvniekerk force-pushed the compilers branch from 227dfbc to c4d7f1f Compare July 15, 2018 21:51

Added build number mangling

b17e1e7

Simplified utility script

isuruf reviewed Jul 18, 2018

View reviewed changes

Changed build number mangling to deal with multiple outputs

ec82dcd

mariusvniekerk added 2 commits July 17, 2018 22:41

fix missing newline

7d458cf

a few small fixes

f22f4df

mariusvniekerk commented Jul 18, 2018

View reviewed changes

Switch to a build number decrement

07415a5

isuruf reviewed Aug 1, 2018

View reviewed changes

Switch to using conda-forge-ci-setup=2

3067232

mariusvniekerk mentioned this pull request Aug 2, 2018

Compiler switch conda-forge/conda-forge-ci-setup-feedstock#19

Merged

mariusvniekerk added 2 commits August 6, 2018 10:34

Point fast_finish to the new location in conda-forge-ci-feedstock

fee92c4

Ensure we pull from the appropriate branch for fast-finish

4f28042

Expand defaults to not clobber everything

8aa6f79

mariusvniekerk changed the title ~~Updates for dual compiler output mode under travis and circle~~ [WIP] Updates for dual compiler output mode under travis and circle Aug 6, 2018

mariusvniekerk added 5 commits August 6, 2018 13:24

Fix missing default for channel_targets

6b7f463

Switch to conda-forge-ci-setup=2 for appveyor

dc07dd3

Add support for CF_COMPILER_STACK

e5e251c

This environment variable is used for rendering variants with the dual compiler stack.

Merge branch 'master' into compilers

a5b681a

Remove unneeded pieces

0d4126d

mariusvniekerk changed the title ~~[WIP] Updates for dual compiler output mode under travis and circle~~ Updates for dual compiler output mode under travis and circle Aug 13, 2018

Merge branch 'master' into compilers

010705b

Add CF_MIN_PY_VER etc in order to help with having a single state for…

64009ea

… pinnings

scopatz merged commit 5ba8b1f into conda-forge:master Aug 14, 2018

jakirkham reviewed Aug 22, 2018

View reviewed changes

benbovy mentioned this pull request Aug 23, 2018

Possible build number issue with conda smithy 3.1.12 #881

Open

mariusvniekerk deleted the compilers branch December 16, 2018 04:14

		@@ -0,0 +1,27 @@
		# This file was generated automatically from conda-smithy. To update this configuration,

		@@ -0,0 +1,30 @@
		# This file was generated automatically from conda-smithy. To update this configuration,
		# update the conda-forge.yml and/or the recipe/meta.yaml.

Updates for dual compiler output mode under travis and circle #836

Updates for dual compiler output mode under travis and circle #836

Conversation

mariusvniekerk commented Jul 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scopatz commented Jul 17, 2018

mariusvniekerk commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusvniekerk commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusvniekerk Jul 18, 2018 • edited

Choose a reason for hiding this comment

isuruf commented Jul 18, 2018

mariusvniekerk commented Jul 19, 2018

jakirkham commented Jul 19, 2018

mariusvniekerk commented Jul 19, 2018 • edited

isuruf commented Jul 19, 2018

jakirkham commented Jul 19, 2018

isuruf commented Jul 19, 2018

jakirkham commented Jul 19, 2018

jakirkham commented Jul 19, 2018

isuruf commented Jul 19, 2018

mariusvniekerk commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusvniekerk Aug 1, 2018 • edited

Choose a reason for hiding this comment

scopatz commented Aug 6, 2018

mariusvniekerk commented Aug 6, 2018

mariusvniekerk commented Aug 13, 2018

scopatz commented Aug 13, 2018

mariusvniekerk commented Aug 13, 2018

scopatz commented Aug 13, 2018

scopatz commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakirkham commented Aug 22, 2018

Choose a reason for hiding this comment

mariusvniekerk commented Jul 15, 2018 •

edited

mariusvniekerk Jul 18, 2018 •

edited

mariusvniekerk commented Jul 19, 2018 •

edited

mariusvniekerk Aug 1, 2018 •

edited