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

Enable Windows build #16

Merged
merged 3 commits into from
Dec 5, 2023
Merged

Enable Windows build #16

merged 3 commits into from
Dec 5, 2023

Conversation

wshanks
Copy link
Contributor

@wshanks wshanks commented Sep 6, 2023

This is just seeing what happens with trial and error for now.

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-webservices
Copy link
Contributor

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.

@wshanks
Copy link
Contributor Author

wshanks commented Sep 8, 2023

Currently this is stuck on compiler is out of heap space (for example here from d8b14ed). Upstream builds in GitHub Actions without error (most recent build log at this time). Looking at the resources listed for Azure Pipelines and GitHub Actions, I see that both should have 2 CPUs and 7 GB of RAM. So the current possibilities that I can think of are:

  • Upstream manages to compile just under the 7 GB limit and something about the conda tooling pushes it over the limit. I am not sure what to do if this is the case.
  • Something about the way I have set up the build is leading to excessive memory usage. I am not sure how to diagnose this, but it would be more hopeful than the previous case.

I looked closely at the build logs mentioned above and I didn't see much difference between here and upstream besides using Visual Studio 17 2022 here and Visual Studio 16 2019 there.

@jaimergp
Copy link
Member

Maybe you can increase the pagefile. I don't know if it's documented (we should!), but you can find this block in the numpy-feedstock:

azure:
  settings_win:
    variables:
      SET_PAGEFILE: 'True'

Add it and rerender to apply it to the pipelines. Let's see 🤞

@wshanks
Copy link
Contributor Author

wshanks commented Sep 14, 2023

Hmm, the build recipe step took two minutes longer, so maybe the pagefile change did something, but the build still ran into the compiler out of heap space message. I am not that familiar with building on Windows. I feel like I must be missing something in the set up because I don't think this should be an excessively large build.

@jaimergp
Copy link
Member

Oh nice, now it does build (there's a missing runtime dep but that should be easy to fix). So the 2019 works but 2022 doesn't? That's interesting.

@wshanks
Copy link
Contributor Author

wshanks commented Sep 15, 2023

So the 2019 works but 2022 doesn't? That's interesting.

Yes, I am not sure why that is, but upstream builds with windows-2019, so I will hope upstream will move to windows-2022 and figure it out before windows-2019 can not be used for conda-forge any more.

there's a missing runtime dep but that should be easy to fix

It's actually that the cmake configuration hardcodes a bundled openblas unless you specify a path to the blas library. I don't think either of those options are viable for conda-forge, so we need to patch in and hopefully get upstream to accept an option not to use the bundled blas.

@wshanks
Copy link
Contributor Author

wshanks commented Sep 15, 2023

It builds now! I included a patch to let cmake use the conda openblas library. I submitted the patch here. If a form of the patch is accepted upstream, I will backport it into the recipe here and release a Windows build (with the intent to remove the patch once it is included in a release).

@wshanks
Copy link
Contributor Author

wshanks commented Sep 15, 2023

@leofang Sorry if I asked you about this already, would you worry about things like this (this one gets warnings on Linux as well)?

WARNING (qiskit-aer): run-exports library package conda-forge::spdlog-1.12.0-h64d2f7d_1 in requirements/run but it is not used (i.e. it is overdepending or perhaps statically linked? If that is what you want then add it to `build/ignore_run_exports`)

recipe/bld.bat Outdated
@@ -0,0 +1,2 @@
SET CMAKE_ARGS="%CMAKE_ARGS% -DUSE_BUNDLED_BLAS_WIN:BOOL=FALSE"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SET CMAKE_ARGS="%CMAKE_ARGS% -DUSE_BUNDLED_BLAS_WIN:BOOL=FALSE"
SET "CMAKE_ARGS=%CMAKE_ARGS% -DUSE_BUNDLED_BLAS_WIN:BOOL=FALSE"

CMD quoting rules are tricky. This is a bit more robust, but you might see issues with nested quotes. Keep an eye.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had nested quotes with the version you commented on, which did not work, but then it worked with no quotes. I will try your suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, is this how you set CMAKE_ARGS on Windows when not invoking cmake directly? scikit-build's documentation suggests using python setup.py ... <cmake args> but I want to keep pip install as more future-proof (not sure if python setup.py still works any way). The alternative way to set CMake options is to use CMAKE_ARGS but conda-forge's build set up overrides CMAKE_ARGS when I try to set it in the recipe, so I moved it down in the script like this, so I could extend what conda-forge set after it set it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, conda-forge infra sets CMAKE_ARGS for you. It looks like the current approach works so... yay? 🥳

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's good now 🙂 I just wasn't sure if dropping to bld.bat and extending CMAKE_ARGS was idiomatic for conda-forge or if there were some other way to extend CMAKE_ARGS.

@wshanks wshanks marked this pull request as ready for review December 4, 2023 21:57
@wshanks wshanks requested a review from leofang as a code owner December 4, 2023 21:57
@wshanks wshanks merged commit 87679dd into conda-forge:main Dec 5, 2023
10 checks passed
@wshanks wshanks deleted the win branch December 5, 2023 00:55
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.

2 participants