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

3.1.0 from git source #173

Closed

Conversation

tomjakubowski
Copy link
Contributor

@tomjakubowski tomjakubowski commented Oct 8, 2024

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.

This switches to a git-source build mimicking as closely as possible the build done in perspective's CI.

This includes a number of patches which I will work on upstreaming to perspective:

  • remove various postinstall hooks (maybe not desirable upstream)
  • use header-only boost
  • support non-abi3 Python build (until CEP: support for ABI3 packages conda/ceps#86 is accepted)
  • explicit selection of python interpreter for Maturin
  • use conda-provided protoc (protobuf-src crate doesn't build in cross-compiling conda environment)
  • patch arrow dependency to solve libtool issue on cross-compiling mac builders
  • patch perspectve-server's cmake invocation to use CMAKE_ARGS from environment

tomjakubowski and others added 4 commits October 7, 2024 20:48
includes a number of patches which we will attempt upstreaming to
perspective:
- remove various postinstall hooks (maybe not upstreamable)
- use header-only boost
- support non-abi3 build
- explicit selection of python interpreter for maturin
- use conda-provided protoc (protobuf-src crate doesn't build in
  cross-compiling conda environment)
@tomjakubowski
Copy link
Contributor Author

@conda-forge-admin, please rerender

@conda-forge-admin
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/meta.yaml) and found it was in an excellent condition.

@tomjakubowski
Copy link
Contributor Author

tomjakubowski commented Oct 8, 2024

Re-rendering added a whole new dimension now in the build matrix for Node.JS version: 20 or 22. I guess this is because nodejs was added as a build dependency.

The build artifacts should be indifferent to the installed Node.JS version - Node is only used to orchestrate cmake and maturin. I'd like to understand better if we should remove this extra Node.JS dimension or keep it. Node.JS version shouldn't matter for users of the package, but it is still perhaps useful to verify in CI that the package successfully builds with newer Node.JS versions. Digesting all of this now https://docs.conda.io/projects/conda-build/en/stable/resources/variants.html

As for the jupyter extensions, the plan is to fix upstream issues in the sdist, and then update the recipe here to pull from that. That may come in 3.1.1.

@tomjakubowski
Copy link
Contributor Author

@conda-forge-admin, please rerender

@tomjakubowski
Copy link
Contributor Author

build/force_ignore_keys: [nodejs] removed nodejs from the build hash. I tried another way, which was better-attested, by specifying nodejs: [20] and ignore_version: [nodejs] in conda_build_config.yaml, but local rerenders with that change failed to update the .ci_support files. I'm probably misunderstanding something, will look at the conda-smithy source tomorrow.

it's sufficient to depend on pnpm, which brings in nodejs as its own
dependency

this prevents nodejs from appearing in perspective-python's build hash
@tomjakubowski
Copy link
Contributor Author

tomjakubowski commented Oct 8, 2024

Landed on this: removed nodejs as a build dependency -- it comes along with pnpm anyhow -- and that takes it out of the build hash and matrix.

Expecting the next re-render may not change anything

@tomjakubowski
Copy link
Contributor Author

@conda-forge-admin, please rerender

@conda-forge-admin
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.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/11228305885.

@tomjakubowski tomjakubowski force-pushed the 3.1.0-from-git-source branch from 2eb6940 to 3a5ae0f Compare October 8, 2024 20:22
@tomjakubowski tomjakubowski marked this pull request as ready for review October 8, 2024 20:28
@tomjakubowski tomjakubowski force-pushed the 3.1.0-from-git-source branch from 01c32d5 to 2469a75 Compare October 9, 2024 00:11
@tomjakubowski
Copy link
Contributor Author

Verified the license file is being packaged:

@tomjakubowski ➜ /workspaces/3.0.1 $ tar tvf ../pkg-perspective-python-3.1.0-py312h98163a5_0.tar.zst | grep -i license
-rw-rw-rw- 0/0           10541 2024-10-09 17:26 info/licenses/LICENSE.md

Also discovered while doing this that the 3.0.1 packages contain only the license file (note the absence of a pipe to grep). Makes sense, they're only about a dozen kB: https://anaconda.org/conda-forge/perspective-python/files

@tomjakubowski ➜ /workspaces/3.0.1 $ tar tvf pkg-perspective-python-3.0.1-py311h7df6f3f_1.tar.zst 
-rw-rw-r-- 0/0           10541 2024-08-26 02:52 info/licenses/perspective-cpp/LICENSE.md

@tomjakubowski
Copy link
Contributor Author

tomjakubowski commented Oct 9, 2024

Silly me - the bulk of the package (including the .py files) was previously in the perspective package; the 2.10 and 3.0 perspective-python packages are (virtually) empty. This PR changes things and combines them into perspective-python

recipe/meta.yaml Outdated Show resolved Hide resolved
@tomjakubowski
Copy link
Contributor Author

@conda-forge-admin, please rerender

conda-forge-webservices[bot] and others added 2 commits October 10, 2024 00:21
@tomjakubowski tomjakubowski marked this pull request as draft October 12, 2024 00:55
@tomjakubowski tomjakubowski marked this pull request as ready for review October 12, 2024 00:56
@tomjakubowski
Copy link
Contributor Author

tomjakubowski commented Oct 14, 2024

Locally, mac builds with ARROW_DEPENDENCY_SOURCE=BUNDLED are building with references to jemalloc symbols in libarrow.a, despite Arrow's build reporting ARROW_JEMALLOC=OFF. This causes link errors from the .so on importing perspective, which trips the import test. I can also replicate this misconfigured build in perspective outside of conda, when I apply the patch to use ARROW_DEPENDENCY_SOURCE=BUNDLED.

I don't know what this is yet, but it does appear that the import tests are succeeding in CI, so it is possible it is something particular about my local setup. May try someone else as a guinea pig

edit: this turned out to be caused by perspective's build of arrow picking up #include "arrow/config.h" from /usr/local/include instead of the Arrow sources. I had installed Arrow to /usr/local ages ago, and the config.h had ARROW_JEMALLOC defined. Removing Arrow from /usr/local fixed my local perspective builds. Another sign IMO to do a cleanup pass on perspective's cmakelists

- GIT_REPOSITORY https://github.com/apache/arrow.git
- GIT_TAG apache-arrow-17.0.0
+ GIT_REPOSITORY https://github.com/ProspectiveCo/arrow.git
+ GIT_TAG tom-libtool-fix2
Copy link
Member

Choose a reason for hiding this comment

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

No

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That tag includes a patch to work around an obscure issue in Arrow when building it with bundled dependencies when /usr/bin/libtool is broken (as it seems to be on the conda-forge cross-compiling Mac builders).

The bespoke tag on the Prospective fork was an expedient way to test that change turned builds green. Aside from removing the Mac builders and waiting for an arrow release which includes apache/arrow#44385, the alternative is to patch arrow.txt.in to add a PATCH_COMMAND which includes the arrow patch. I'll do that.

Copy link
Contributor Author

@tomjakubowski tomjakubowski Oct 16, 2024

Choose a reason for hiding this comment

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

Patch isn't sufficient, need to make perspective's build read CMAKE_ARGS from the environment too. In the python build, cmake is invoked from perspective-server's build script using the cmake crate. This is where it gets rocky: the cmake crate doesn't support passing arbitrary arguments to cmake. As of 2017 they said they'd accept a PR to add the feature. open issue

Two ways occur to me solve this:

  • parse CMAKE_ARGS in the Rust build script from the environment and turn each parsed arg into a method call on the cmake::Config builder object, e.g. cmake.define(var, var), which would set the correct value for CMAKE_LIBTOOL
  • parse CMAKE_ARGS from the environment in CMakeLists.txt and turn them intoset(${var} ${value}) and other cmake calls

could also side-step this and just patch the the cmake invocation in FindInstallDependency.cmake to pass on -DCMAKE_LIBTOOL=$ENV{LIBTOOL} but I would rather make perspective's build respect all CMAKE_ARGS if it isn't too much work

edit: discovered that, in fact, the cmake crate does support arbitrary cmake args with configure_arg(). I'm using that with a crude split-on-spaces to parse CMAKE_ARGS. Upstream's fix can use shlex to parse that variable (like arrow does), going the crude way here because it looks like it will work in CI and it avoids patching or updating Cargo.toml/lock in the build. If escapes or spaces are really needed then I'll capitulate and patch shlex in

tomjakubowski and others added 4 commits October 15, 2024 18:31
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