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

Implement name-like search paths #31

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dcbaker
Copy link
Collaborator

@dcbaker dcbaker commented Mar 5, 2024

With this set of changes search paths are implemented for name-like paths, this includes:

  • prefix/subdir/cps/name.cps
  • prefix/subdir/cps/name/*.cps

The current version has been completely rewritten from the original version of this PR, as much of the underlying problems I encountered have since been fixed by changes to the spec or by other PRs to cps-config itself.

Copy link
Collaborator

@bretbrownjr bretbrownjr left a comment

Choose a reason for hiding this comment

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

I appreciate the additional test cases a lot!

I might need some time with an IDE to fully digest this change, so I'll call this a neutral set of comments.

Probably @mwoehlke-kitware should take a look too. If he wants to approve this change, don't wait on me to land. I can circle back later with my own PRs.

src/platform.cpp Outdated Show resolved Hide resolved
src/platform.cpp Outdated Show resolved Hide resolved
src/platform.hpp Outdated Show resolved Hide resolved
src/search.cpp Outdated Show resolved Hide resolved
src/search.cpp Outdated Show resolved Hide resolved
src/search.cpp Outdated Show resolved Hide resolved
tests/cases/lib/cps/cps-path-not-set-root.cps Outdated Show resolved Hide resolved
@dcbaker dcbaker force-pushed the submit/name-like-searches branch 4 times, most recently from 179b6cb to bb1055c Compare March 8, 2024 22:18
Copy link
Collaborator

@bretbrownjr bretbrownjr left a comment

Choose a reason for hiding this comment

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

Looks good to merge.

Let's make sure we circle back on the lowercase attributes and come up with a plan for TODO hygiene.

@dcbaker
Copy link
Collaborator Author

dcbaker commented Mar 10, 2024

Rebased on main, lets indeed circle back on some of the hygene stuff, I think it might be appropriate to have an issue or two around some of that in the cps-config repo

@dcbaker dcbaker mentioned this pull request Mar 13, 2024
@dcbaker dcbaker force-pushed the submit/name-like-searches branch 5 times, most recently from 1219010 to 063501a Compare March 15, 2024 20:11
@dcbaker
Copy link
Collaborator Author

dcbaker commented Mar 15, 2024

I've rebased on top of the other great work going on, and updated the CPS_PATH and CPS_PREFIX_PATH to match what the spec has upstream as of now.

@dcbaker dcbaker force-pushed the submit/name-like-searches branch 3 times, most recently from 7f0627c to 8acd27a Compare March 15, 2024 20:45
@bretbrownjr
Copy link
Collaborator

@dcbaker I'm just looking through the PR backlog. Is this ready to review? Can we put PRs in Draft state otherwise so I know they're not ready?

@dcbaker
Copy link
Collaborator Author

dcbaker commented Mar 18, 2024

I think this is ready for review

Copy link
Collaborator

@bretbrownjr bretbrownjr left a comment

Choose a reason for hiding this comment

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

At a minimum, we'll want an intentional assertion failure or something.

Copy link
Collaborator

@bretbrownjr bretbrownjr left a comment

Choose a reason for hiding this comment

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

Mainly need:

  • Error handling on undefined environment variables (i.e., nullptr handling)
  • Test cases for undefined environment variables, at least to prevent crashes
  • Consistent use of CPS_CONFIG_LIBDIR_NAME instead of CPS_LIBDIR
  • Test cases for CPS_CONFIG_LIBDIR_NAME

src/cps/meson.build Outdated Show resolved Hide resolved
src/cps/platform.cpp Outdated Show resolved Hide resolved
src/cps/platform.hpp Outdated Show resolved Hide resolved
src/cps/search.cpp Outdated Show resolved Hide resolved
src/cps/env.cpp Outdated Show resolved Hide resolved
@dcbaker dcbaker force-pushed the submit/name-like-searches branch 2 times, most recently from 0173c9a to 6d1cf3d Compare September 25, 2024 23:07
This adds support for finding cps files in `$ROOT/$name/*.cps`, as well
as files with capitalization.
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