Skip to content

Conversation

@ll-nick
Copy link
Collaborator

@ll-nick ll-nick commented Nov 29, 2024

This tidies up the readme in the style we use in the arbitration_graphs repo.
This moves large parts of the existing readme into collapsible spoilers, adds shield.io badges, and a list of features.

#patch

@ll-nick ll-nick requested a review from orzechow November 29, 2024 09:35
@ll-nick ll-nick self-assigned this Nov 29, 2024
Copy link
Member

@orzechow orzechow 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 improvements!

I like the feature list of course 😁
Found one mistake though that needs improvement.

README.md Outdated
```

The library is installed in the Docker image under `/usr/local/include/util_caching/` and `/usr/local/lib/cmake/util_caching/`.
The library is installed in the Docker image under `/usr/include/util_caching/` and `/usr/lib/cmake/util_caching/`.
Copy link
Member

@orzechow orzechow Dec 5, 2024

Choose a reason for hiding this comment

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

That's probably not correct for the docker image, as we haven't set CMAKE_INSTALL_PREFIX here (as we did in arbitration_graphs).

Thus, the install location of util_caching is different for

  • a local install via cmake/make (as in our last docker stage)
    /usr/local/include/util_caching
  • and a .deb archive install
    /usr/include/util_caching

I'd avoid forcing the install prefix in the CMakeLists.txt, as was necessary for the arbitration_graph GUI…
We could set it in the Docker though:

RUN cmake -DCMAKE_INSTALL_PREFIX="/usr" .. && \
    cmake --build . && \
    cmake --install . && \
    rm -rf /tmp/util_caching

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I see. I thought it was just outdated because I have the .deb version installed locally. I don't think forcing another path is necessary. I corrected the path in 1988385.

@ll-nick ll-nick requested a review from orzechow December 6, 2024 10:18
@ll-nick ll-nick force-pushed the make_readme_great_again branch from 1988385 to 636b89d Compare December 6, 2024 10:58
@ll-nick
Copy link
Collaborator Author

ll-nick commented Dec 6, 2024

I just rebased on main and integrated the parts about the python bindings into the feature list and the usage documentation.

Copy link
Member

@orzechow orzechow left a comment

Choose a reason for hiding this comment

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

Great! 🐍

@ll-nick ll-nick merged commit e7b2d8e into main Dec 6, 2024
1 check passed
@ll-nick ll-nick deleted the make_readme_great_again branch December 6, 2024 19:49
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