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

WIP: Bump fmt (to 10.2.1) and spdlog (to 1.13), remove patches #592

Closed
wants to merge 16 commits into from

Conversation

jameslamb
Copy link
Member

@jameslamb jameslamb commented Apr 24, 2024

Description

Contributes to:

Makes the following direct changes to cpm/versions.json

  • bumps fmt (10.1.1 -> 10.2.1), removes patches
  • bumps spdlog (1.12.0 -> 1.13.0), removes patches

Which led to the following supporting changes:

  • modifies pinning tests from rapids-cmake can generate pinned versions file #530:
    • to only test projects that were downloaded by CPM (e.g. ignoring the fmt that already exists in the build environment)
    • to echo out pinned_versions.json and versions.json in logs from failed tests, to make debugging faster

Notes for Reviewers

Just opening this to test. Will fill this out more when it's ready for review.

Related:

How I tested this

Pushed a commit with the new test error message content changes but keeping fmt in the failing tests, to confirm that the expected tests failed.

got the expected outputs (click me)
The following tests FAILED:
	698 - cpm_generate_pins-nested-makefile (Failed)
	700 - cpm_generate_pins-nested-ninja (Failed)
	702 - cpm_generate_pins-nested-ninja_multi-config (Failed)
	722 - cpm_generate_pins-simple-makefile (Failed)
	724 - cpm_generate_pins-simple-ninja (Failed)
	726 - cpm_generate_pins-simple-ninja_multi-config (Failed)

And they failed in the expected way more informative logs!

CMake Error at CMakeLists.txt:51 (message):
  pinned fmt tag (10.2.1) should differ compared to baseline 10.2.1

  pinned_versions.json:

  {

    "always_download" : true,
    "git_shallow" : false,
    "git_tag" : "${version}",
    "git_url" : "https://github.com/fmtlib/fmt.git",
    "version" : "10.2.1"

  }

  versions.json:

  {

    "git_tag" : "${version}",
    "git_url" : "https://github.com/fmtlib/fmt.git",
    "version" : "10.2.1"

  }

(build link)

Opened PRs on a few repos, pointed at this rapids-cmake branch using the approach described in the rapids-cmake docs (link).

I also modified each of those PRs so that their conda builds and tests consumed CI artifacts from the other PRs. Following this example: rapidsai/cuml#5640.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • The cmake-format.json is up to date with these changes.
  • I have added new files under rapids-cmake/
    • I have added include guards (include_guard(GLOBAL))
    • I have added the associated docs/ rst file and update the api.rst

@jameslamb jameslamb added breaking Introduces a breaking change improvement Improves an existing functionality 2 - In Progress Currenty a work in progress labels Apr 24, 2024
if(${proj}_SOURCE_DIR)
list(APPEND projects-to-verify ${proj})
endif()
endforeach()
Copy link
Member Author

Choose a reason for hiding this comment

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

I chose not to add validation here that projects-to-verify is non-empty. That already happens in the verify/ scripts.

e.g.

string(REPLACE " " ";" projects-to-verify "${projects-to-verify}")
if(NOT projects-to-verify)
message(FATAL_ERROR "Failed to pass anything in projects-to-verify")
endif()

rapids-bot bot pushed a commit that referenced this pull request May 1, 2024
Modifies pinning tests from #530:
* to only test projects that were downloaded by CPM (e.g. ignoring the `fmt` that might already exist in the build environment)
* to echo out `pinned_versions.json` and `versions.json` in logs from failed tests, to make debugging faster

## Notes for Reviewers

#592 proposes some testing changes that aren't specific to the goals of that PR.

Since that PR might be stuck for a bit (rapidsai/build-planning#56 (comment)), this proposes pulling those out into a separate PR:

* so that other changes in this project benefit from them
* to shrink the diff of #592 and therefore the risk of merge conflicts

### How I tested this

Pushed a commit with the new test error message content changes but keeping `fmt` in the failing tests, to confirm that the expected tests failed.

<details><summary>got the expected outputs (click me)</summary>

```text
The following tests FAILED:
	698 - cpm_generate_pins-nested-makefile (Failed)
	700 - cpm_generate_pins-nested-ninja (Failed)
	702 - cpm_generate_pins-nested-ninja_multi-config (Failed)
	722 - cpm_generate_pins-simple-makefile (Failed)
	724 - cpm_generate_pins-simple-ninja (Failed)
	726 - cpm_generate_pins-simple-ninja_multi-config (Failed)
```

And they failed in the expected way more informative logs!

```text
CMake Error at CMakeLists.txt:51 (message):
  pinned fmt tag (10.2.1) should differ compared to baseline 10.2.1

  pinned_versions.json:

  {

    "always_download" : true,
    "git_shallow" : false,
    "git_tag" : "${version}",
    "git_url" : "https://github.com/fmtlib/fmt.git",
    "version" : "10.2.1"

  }

  versions.json:

  {

    "git_tag" : "${version}",
    "git_url" : "https://github.com/fmtlib/fmt.git",
    "version" : "10.2.1"

  }
```

([build link](https://github.com/rapidsai/rapids-cmake/actions/runs/8837613213/job/24267079292?pr=592))

</details>

#

Authors:
  - James Lamb (https://github.com/jameslamb)

Approvers:
  - Robert Maynard (https://github.com/robertmaynard)
  - Kyle Edwards (https://github.com/KyleFromNVIDIA)

URL: #599
@jameslamb jameslamb changed the base branch from branch-24.06 to branch-24.08 May 21, 2024 14:42
@robertmaynard robertmaynard mentioned this pull request May 23, 2024
7 tasks
@jameslamb
Copy link
Member Author

This work is paused, in favor of pursuing a better long-term solution in the future. Closing this PR for now.

Subscribe to rapidsai/build-planning#54 and rapidsai/build-planning#56 for updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 - In Progress Currenty a work in progress breaking Introduces a breaking change improvement Improves an existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant