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

Try to trigger new boost build #123

Merged
merged 10 commits into from
Dec 22, 2020
Merged

Try to trigger new boost build #123

merged 10 commits into from
Dec 22, 2020

Conversation

jan-janssen
Copy link
Member

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@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 (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

@jan-janssen
Copy link
Member Author

@conda-forge-admin, please rerender

conda-forge-linter and others added 2 commits December 22, 2020 09:22
@jan-janssen
Copy link
Member Author

@conda-forge-admin, please rerender

@github-actions
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.
I tried to rerender for you, but it looks like there was nothing to do.

@jan-janssen
Copy link
Member Author

@conda-forge-admin, please rerender

@github-actions
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.
I tried to rerender for you, but it looks like there was nothing to do.

@mbargull
Copy link
Member

@conda-forge-admin, please rerender

@@ -164,8 +164,7 @@ outputs:
- suitesparse
- zlib
- fenics-ffc =={{ version }}
- hdf5 * {{ mpi_prefix }}_*
- {{ pin_compatible("boost-cpp", exact=True) }}
Copy link
Member

Choose a reason for hiding this comment

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

{{ pin_compatible(..., exact=True) }} means that it pins to the exact build (i.e., on the build string level).
Here you rather want a plain boost-cpp in host/run so that conda-build will replace it with the pinnings definition.

Copy link
Member Author

Choose a reason for hiding this comment

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

The fenics package requires the exact boost-cpp version which is the cause of conda-forge/mshr-feedstock#18

Copy link
Member

Choose a reason for hiding this comment

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

exact boost-cpp version

Exact version would be understandable. But the current conda-forge-pinning already has a max_pin: x.x.x for boost-cpp which means if you have it in host and run, then you'd get a pin to >=1.72.0,<1.73.0a0.
Exact build is what doesn't seem sensible to me.

Copy link
Member

Choose a reason for hiding this comment

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

If you don't trust conda-forge-pinning (e.g., to avoid a max_pin change affecting this recipe), you can do {{ pin_compatible("boost-cpp", max_pin="x.x.x") }} yourself.

@@ -164,8 +164,7 @@ outputs:
- suitesparse
- zlib
- fenics-ffc =={{ version }}
- hdf5 * {{ mpi_prefix }}_*
Copy link
Member

Choose a reason for hiding this comment

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

hdf5 has a run_export has includes the MPI variant. We can rely on that one.

@mbargull
Copy link
Member

mbargull commented Dec 22, 2020

@jan-janssen, I don't know why conda-smithy splits the build jobs for osx for each boost-cpp. But the linux build also creates two (boost-cpp-dependent) builds for fenics-libdolfin. Any downstream issue you encountered was probably due to the exact=True pin.

@jan-janssen
Copy link
Member Author

The reason why we need to pin boost explicitly is conda-forge/mshr-feedstock#18 and as you removed my changes we are basically back at #120 .

I know that adding boost-cpp is a work around but to me it seems like a reasonable solution and I would like to try if it fixes conda-forge/mshr-feedstock#23

@jan-janssen
Copy link
Member Author

As the tests take an hour to run I opened another pull request with my previous suggestion #124

@jan-janssen
Copy link
Member Author

@jan-janssen, I don't know why conda-smithy splits the build jobs for osx for each boost-cpp. But the linux build also creates two (boost-cpp-dependent) builds for fenics-libdolfin. Any downstream issue you encountered was probably due to the exact=True pin.

It seems to be an issue with combined recipes. When there is just the lib package it creates two builds for linux as well as it should. The easiest way I was able to force two builds was by adding boost-cpp to each sub package.

@mbargull
Copy link
Member

The reason why we need to pin boost explicitly is conda-forge/mshr-feedstock#18 and as you removed my changes we are basically back at #120 .

I know that adding boost-cpp is a work around but to me it seems like a reasonable solution and I would like to try if it fixes conda-forge/mshr-feedstock#23

That workaround is no good, because the pinning is too specific and is likely to cause downstream (build and user-site) issues.
conda-forge/mshr-feedstock#18 is about a problem with a boost-cpp version change from 1.70 to 1.72. I don't see any indication that the exact boost-cpp build is of significance there.

@jan-janssen, I don't know why conda-smithy splits the build jobs for osx for each boost-cpp. But the linux build also creates two (boost-cpp-dependent) builds for fenics-libdolfin. Any downstream issue you encountered was probably due to the exact=True pin.

It seems to be an issue with combined recipes. When there is just the lib package it creates two builds for linux as well as it should. The easiest way I was able to force two builds was by adding boost-cpp to each sub package.

The behavior for Linux I understand. It should be fine to build multiple variants in one jobs.
conda-smithy's behavior macOS I don't understand.

Comment on lines +33 to +40
# NOTE: Top-level environment with boost-cpp is only to separate the build for
# multiple boost versions into different CI jobs.
# TODO: Needs investigation why "separate the two boost builds more strictly"
# would be necessary. Ref:
# https://github.com/conda-forge/mshr-feedstock/pull/23#issuecomment-749520383
requirements:
host:
- boost-cpp
Copy link
Member

Choose a reason for hiding this comment

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

@jan-janssen, since you weren't happy with my changes, I added this.
This is still just a workaround. I don't know if it's necessary. If it is, the cause should be investigated.
Prime andidates for causes are always:

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks

@mbargull
Copy link
Member

@conda-forge-admin, please rerender

@jan-janssen
Copy link
Member Author

@mbargull I am fine with the changes - I was not aware of the requirements in addition to outputs. So if you agree I would suggest to merge this one once the tests passed.

@mbargull
Copy link
Member

I was not aware of the requirements in addition to outputs.

Yeah, conda-build has a build stage before the builds of the outputs take place. Which means you can create a build/host env combo in which build.sh/bld.bat (if existing) will be run. Here we just create and don't use it -- meaning it's useless, but it forced boost-cpp to be in the top-level variant matrix to persuade conda-smithy to split the jobs.

So if you agree I would suggest to merge this one once the tests passed.

Sure, go ahead if it looks fine to you! Just trying to help with avoiding subsequent issues here ;).

@mbargull
Copy link
Member

If we want to be super-correct, it looks like boost-cpp should also be a host+run dependency for fenics-dolfin: https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=255038&view=logs&j=9a864fd9-6c8f-52ca-79ce-2aa6dca1a1de&t=10fc5aa2-324e-5982-4c88-6b31fcab16b3&l=3300
, i.e., it's a direct dependency.
(In practice, due to the {{ pin_subpackage("fenics-libdolfin", exact=True) }}, the dep will be correctly installed.)

@jan-janssen, you seemed to be time-constrained here, so I'll leave this up to you if you want to fix this now or rather later.

@jan-janssen
Copy link
Member Author

@jan-janssen, you seemed to be time-constrained here, so I'll leave this up to you if you want to fix this now or rather later.

I open another pull request for this and merge this one so we can continue with conda-forge/mshr-feedstock#23

@jan-janssen jan-janssen merged commit 697f470 into conda-forge:master Dec 22, 2020
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.

3 participants