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

CI sdist fixes, port conda patch, fix ODR violation #2826

Merged
merged 5 commits into from
Nov 6, 2024

Conversation

tomjakubowski
Copy link
Contributor

@tomjakubowski tomjakubowski commented Nov 5, 2024

Follow ups for #2811

  • Make the new test_python_sdist run only on release tags
  • Remove vcpkg from Windows install-deps action
  • Fix some cmake issues that broke the Windows conda build

fixes bug when cmake is installed to a path with a space in it

Signed-off-by: Tom Jakubowski <[email protected]>
Also updates the cargo patch-job so it uses the .crate files produced
earlier in the workflow instead of using git sources with metadata. This
is closer to what happens when building and installing from a release
sdist, which pulls .crate files from crates.io.

Signed-off-by: Tom Jakubowski <[email protected]>
Change how the CMAKE_C_FLAGS_RELEASE and CMAKE_CXX_FLAGS_RELEASE
variables are reset and used.  They are now interpolated into
CMAKE_CXX_FLAGS, placed before the EXTENDED_FLAGS so that EXTENDED_FLAGS
and OPT_FLAGS can override the FLAGS_RELEASE.  This ensures that RELEASE
flags from the environment are used, while allowing them to be
overridden by perspective's optimization flags.

Also preserve values of `CMAKE_CXX_FLAGS` set by the builder.

This is a port of a conda-feedstock patch by Isuru Fernando.

<conda-forge/perspective-feedstock@9eab18c>

Signed-off-by: Tom Jakubowski <[email protected]>
Signed-off-by: Tom Jakubowski <[email protected]>
this was an ODR violation: the same function was defined twice, once
with `inline` specifier in utils.h, and once without `inline` in
utils.cpp

it happened to work on clang and gcc and msvc, most of the time, but
broke in the conda build on Windows

Signed-off-by: Tom Jakubowski <[email protected]>
@tomjakubowski
Copy link
Contributor Author

test tag CI run, with a simulated 3.1.4 release commit: https://github.com/tomjakubowski/perspective/actions/runs/11677140432

@tomjakubowski tomjakubowski marked this pull request as ready for review November 5, 2024 04:52
@tomjakubowski
Copy link
Contributor Author

tomjakubowski commented Nov 5, 2024

Oneoff CI run which tests my WIP branch updating the perspective-feedstock repo to v3.1.4. The CI job uses perspective-client.crate and perspective-server.crate artifacts from the test tag build above. The build is using an sdist from a previous test tag run which I uploaded somewhere https://github.com/tomjakubowski/perspective-oneoff-tests/actions/runs/11678364614/job/32517697170

it wouldn't be too much trouble to update that workflow to instead download the sdist artifact from the same run it gets the .crates from. In short:

  1. Download the sdist artifact
  2. Glean the version number from the sdist's filename and replace it in meta.yaml
  3. Replace URL in meta.yaml with file:// URL pointing to sdist artifact

Then that workflow would be a decent way to sanity check future builds against conda (maybe with more platform coverage). Better still maybe to trigger (augmented versions of?) perspective-feedstock's workflows from our CI, supplying .crate and sdist artifacts from our workflows.

Copy link
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Looks good!

@texodus texodus merged commit b9af3bd into finos:master Nov 6, 2024
13 checks passed
@texodus texodus added the internal Internal refactoring and code quality improvement label Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Internal refactoring and code quality improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants