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

Adopting utl::log() #165

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Lucas-C
Copy link

@Lucas-C Lucas-C commented Jan 17, 2025

No description provided.

@felixguendling
Copy link
Member

I think we need an option in utl to enable the docs.
UTL_BUILD_DOCS with default value OFF.

And then wrap this in if (UTL_BUILD_DOCS):
https://github.com/motis-project/utl/blob/69e700e03ae3d385304d754cf57877528bc591ef/CMakeLists.txt#L33

@Lucas-C Lucas-C force-pushed the adopt-utl-log branch 2 times, most recently from f72c943 to d795837 Compare January 17, 2025 14:44
@felixguendling
Copy link
Member

Seeing the code, maybe it makes sense to rename to something with log in the name (e.g. utl::log_err)? At least utl::error could also be an exception class derived from std::exception.

@Lucas-C
Copy link
Author

Lucas-C commented Jan 17, 2025

I think we need an option in utl to enable the docs. UTL_BUILD_DOCS with default value OFF.

And then wrap this in if (UTL_BUILD_DOCS): motis-project/utl@69e700e/CMakeLists.txt#L33

Do all the MOTIS repos builds rely on the CMakeLists.txt in the utl module?

I'm not a big CMake expert, so I'm not sure how best to share the doc-building logic between MOTIS repositories.
I think the following could be shared from motis-project/utl:

  • docs/CMakeLists.txt
  • docs/Doxyfile (except from the module name)
  • docs/conf.py (except from the module name)
  • docs/motis-logo.svg
  • docs/requirements.txt
  • the doc job from .github/workflows/unix.yml

Maybe this should be tackled in a separate issue / PR?

@Lucas-C
Copy link
Author

Lucas-C commented Jan 17, 2025

Seeing the code, maybe it makes sense to rename to something with log in the name (e.g. utl::log_err)? At least utl::error could also be an exception class derived from std::exception.

Yes, I agree, I think it would be clearer.

I opened PR motis-project/utl#32 to add this prefix.

@felixguendling
Copy link
Member

felixguendling commented Jan 17, 2025

I think we need an option in utl to enable the docs. UTL_BUILD_DOCS with default value OFF.
And then wrap this in if (UTL_BUILD_DOCS): motis-project/utl@69e700e/CMakeLists.txt#L33

Do all the MOTIS repos builds rely on the CMakeLists.txt in the utl module?

I'm not a big CMake expert, so I'm not sure how best to share the doc-building logic between MOTIS repositories. I think the following could be shared from motis-project/utl:

  • docs/CMakeLists.txt
  • docs/Doxyfile (except from the module name)
  • docs/conf.py (except from the module name)
  • docs/motis-logo.svg
  • docs/requirements.txt
  • the doc job from .github/workflows/unix.yml

Maybe this should be tackled in a separate issue / PR?

In case this docs building is always the same and should be used in all MOTIS repositories (osr, motis, etc.), I would propose to create a separate repository (maybe called docs-gen or simply docs) and include it via .pkg. As long as it has a central CMakeLists.txt (then in the root directory) it will work. I think this is maybe a better solution than to force each repo to include utl if it wants only docs. Maybe at the moment all repos include utl but I would say it's maybe better to split it?

@Lucas-C
Copy link
Author

Lucas-C commented Jan 17, 2025

I would propose to create a separate repository (maybe called docs-gen or simply docs) and include it via .pkg.
I think this is maybe a better solution than to force each repo to include utl if it wants only docs.
Maybe at the moment all repos include utl but I would say it's maybe better to split it?

I agree.
I will have to stop working on MOTIS for now, but I'll try to do this next week.
The documentation generation also requires Python and some depdencies to be installed with pip.
I'm not sure how best to do this from CMake, so unless you have some guidance regarding this, I'll search for existing projects doing something similar.

@Lucas-C
Copy link
Author

Lucas-C commented Jan 17, 2025

Comment moved to: motis-project/motis#702

@felixguendling
Copy link
Member

The purpose of tracing in RAPTOR include/nigiri/routing/raptor/debug.h is mainly to be dumped to a text file that can become gigabytes in size for a single earliest arrival query (not even range query!). We neither want this stuff to go OTEL nor do we need timestamps, log levels, etc. here. I would like to keep it fmt::println for this reason. It's for MOTIS developers who need to go very very deep and check millions of routing operations to find the needle in the haystack if results don't match what we expect (e.g. from previous versions, or other implementations).

@Lucas-C
Copy link
Author

Lucas-C commented Jan 21, 2025

I am currently blocked by this issue: motis-project/utl#35

@felixguendling
Copy link
Member

should be fixed by motis-project/utl#36

I am currently blocked by this issue: motis-project/utl#35

Co-authored-by: Felix Gündling <[email protected]>
@Lucas-C Lucas-C force-pushed the adopt-utl-log branch 9 times, most recently from f3a16ac to 88f1648 Compare January 21, 2025 22:24
@Lucas-C
Copy link
Author

Lucas-C commented Jan 21, 2025

I think this is ready

@@ -0,0 +1,19 @@
#pragma once
Copy link
Member

Choose a reason for hiding this comment

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

Why nigiri::scoped_timer if there's utl::scoped_timer?

@felixguendling
Copy link
Member

I think you were right with the function signature. It's cluttering output a lot. Even if it gives additional context, it's maybe not worth to clutter the text output so much.

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.

2 participants