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

Add static constexpr char* names and hand out views for them #402

Merged
merged 4 commits into from
Jun 9, 2023

Conversation

tmadlener
Copy link
Collaborator

@tmadlener tmadlener commented Apr 4, 2023

BEGINRELEASENOTES

  • Add public static constexpr char* type names to the collections and make the getXXXName() methods return string_views to these strings. This is a breaking change to the interface of the collections if you explicitly rely on them being std::string
    • typeName: the full type name of the collection (returned also by getTypeName)
    • valueTypeName: the (immutable) type name of the objects of the collection (returned by getValueTypeName)
    • dataTypeName: the type name of the data PODs (returned by getDataTypeName)
  • Make unittest environment properly use PODIO_SIOBLOCK_PATH
  • USE_EXTERNAL_CATCH2 now can also be set to AUTO to look for a suitable version of Catch2 before falling back and fetching and building it's own version instead of a hard fail.

ENDRELEASENOTES

Addresses some of the things raised in #313 and is also in preparation for other work of making run-time type information available without too many std::string copies

@tmadlener
Copy link
Collaborator Author

This will break DD4hep for example.

@tmadlener
Copy link
Collaborator Author

@faustus123 @nathanwbrei @wdconinc this still needs a bit of work, but potentially as a heads up that this might break things on your end. Would you prefer to have deprecation warnings (as implemented in #404) to flush out uses without breaking things first? Otherwise we might just go ahead and check and fix the packages that we build regularly and introduce everything with a bunch of "synchronized" PRs.

@wdconinc
Copy link
Contributor

I'll give it a shot with #404 and see how much we should change beyond our datamodel_glue.h header. I'm expecting it to be minimal, though.

@nathanwbrei
Copy link
Contributor

Thanks for the heads-up, @tmadlener! It looks like this affects us pretty minimally.

veprbl pushed a commit to eic/EICrecon that referenced this pull request Apr 21, 2023
…618)

This adds the required `auto`s to support both current and future
`podio::getTypeName` signatures.

Ref: AIDASoft/podio#402,
AIDASoft/podio#404
@tmadlener tmadlener mentioned this pull request May 4, 2023
4 tasks
@tmadlener
Copy link
Collaborator Author

@nathanwbrei @wdconinc would you like to have proper deprecation warnigs and then a switch afterwards, or could we switch directly as done in this PR?

@hegner
Copy link
Collaborator

hegner commented May 15, 2023

adding @faustus123 (reported #313 )

@wdconinc
Copy link
Contributor

After eic/EICrecon#618 we shouldn't be affected by this change.

@tmadlener
Copy link
Collaborator Author

tmadlener commented Jun 5, 2023

CI currently fails on anything that tries to load an SIO block library in the environment that has not yet gone through this switch (i.e. EDM4hep from the underlying environment). Apart from a proper plugin mechanism (see #403), I am not sure we can easily do anything against this.

@tmadlener tmadlener linked an issue Jun 5, 2023 that may be closed by this pull request
@tmadlener tmadlener closed this Jun 5, 2023
@tmadlener tmadlener deleted the static-type-info branch June 5, 2023 12:08
@tmadlener tmadlener restored the static-type-info branch June 5, 2023 12:09
@tmadlener tmadlener reopened this Jun 5, 2023
@tmadlener
Copy link
Collaborator Author

With the latest changes to the cmake config, PODIO_SIOBLOCK_PATH should now be properly passed to the unittest environments. This requires cmake >= 3.22 to work with catch test discovery. If that is not met we fall back to not run test discovery, and all unittest will be reported collectively as unittest

@tmadlener
Copy link
Collaborator Author

key4hep based workflows are failing because they can't find a new enough Catch2. We could change

podio/tests/CMakeLists.txt

Lines 130 to 132 in fda9213

if(USE_EXTERNAL_CATCH2)
find_package(Catch2 3 REQUIRED)
else()

so that USE_EXTERNAL_CATCH2 behaves more like use one if you can find a suitable one, and fall back to building your own if you can't instead of simply failing at cmake stage if the discovered one is not suitable.

@hegner
Copy link
Collaborator

hegner commented Jun 9, 2023

I'd prefer that the switch does not imply any surprises of automatic building. That can "nicely" hide misconfigs

@tmadlener
Copy link
Collaborator Author

tmadlener commented Jun 9, 2023

USE_EXTERNAL_CATCH2 now understands AUTO to first look for a suitable external Catch2 and fall back to building its own if none is found. Switched the key4hep workflows to use that for now, so that this can move forward.

except that now we seem to run into: key4hep/key4hep-spack#493

@hegner
Copy link
Collaborator

hegner commented Jun 9, 2023

much better!

@hegner hegner merged commit 930e600 into AIDASoft:master Jun 9, 2023
@tmadlener tmadlener deleted the static-type-info branch June 12, 2023 08:04
veprbl added a commit to veprbl/JANA2 that referenced this pull request Aug 13, 2023
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.

static version of getValueTypeName() and friends
4 participants