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

UsdCat: Initial conversion to C++ #2090

Merged
merged 4 commits into from
Dec 10, 2022

Conversation

dgovil
Copy link
Collaborator

@dgovil dgovil commented Nov 14, 2022

Description of Change(s)

This PR is an initial conversion of the USD command line tools to C++ so that Python is not a dependency any longer.
This PR only includes usdcat as it had the best test coverage, and was the most common tool to run.

I would like to get buyoff on the build changes in this PR before submitting follow up PRs for usdzip and usdtree that I need to refactor to fit in the USD repo.

Some details:

  • Uses CLI11 as a vendored dependency of the repo. This is a single header C++ library that is compatible with the current C++14 standard used by USD. The code is written in such a way that we can swap out to different parsers in the future.
  • Adds a PXR_USE_PYTHON_CLI_TOOLS argument in CMake that is also exposed to the build_usd.py script to allow people to opt back to the Python utilities. The default is to build the C++ commands, but if specified this will also print a deprecation warning during the CMake generation.
  • Adds some common.h/cpp files in the bin folder for common functions and utilities required by the commands. We can move these to a Tf module somewhere if that's more preferable. Right now I only have the one version formatter, but other commands might add more.
  • Added a SOURCES argument to pxr_cpp_bin to allow passing in other files to the target generation.
  • Passes the existing tests (at least on my end)

Otherwise, this is a fairly standard port of usdcat to C++.

  • [ X] I have verified that all unit tests pass with the proposed changes
  • [ X] I have submitted a signed Contributor License Agreement

@sunyab
Copy link
Contributor

sunyab commented Nov 19, 2022

Filed as internal issue #USD-7775

@spiffmon
Copy link
Member

Hey @dgovil - this is awesome! We have some higher-level feedback that we wanted to give you ASAP (just gathered today). I'll make a separate post for each topic to facilitate discussion.

@spiffmon
Copy link
Member

spiffmon commented Nov 23, 2022

On CLI11:

  • Firstly, we think it might be advantageous in several ways to make the addition of CLI11 be its own PR, which we fast-track first
  • The CLI11 generator script allows you to specify a namespace, and we would like it to use PXR_NS::CLI
  • (This will get repeated later) We do not allow includes/linking from within the bin branches of our repos. CLI11 seems approppriate to us to loft into pxr/base/tf/pxrCLI11
  • The accompanying README.md should include a software hash that is fully describes the version of cli11 used to generate the header we are enshrining
  • While it is OK to have the LICENSE.md in the cli11 directory also, our practice is to enumerate all third-party licenses in the repo's top-level LICENSE.txt file

@spiffmon
Copy link
Member

We haven't dug in deeply enough yet to understand the need for common.h, but again, no includable files in bin. If the enum really needs to be shareable, let's add it in usdUtils.

@spiffmon
Copy link
Member

More broadly on shareability, our preference would be to start out with these things as all being simply executables. If you have a really compelling use-case for having the functionality be shareable/linkable, we can discuss adding to usdUtils, and having binaries be thin arg-parsers that call into usdUtils - but again, our preference would be to keep it simple for now.

@spiffmon
Copy link
Member

We understand there's didactic value in the python code, and we will take a task of creating a tutorial that shows how to "do the stuff that usdcat does"...

@spiffmon
Copy link
Member

But that said, we think that the utilities have good enough test coverage (exceptions already noted in private email, I think?) that we would rather immediately kill the python versions, thus also keeping the build simple and not needing to worry about duplicate implementations.

That's it for now, thanks!

@dgovil
Copy link
Collaborator Author

dgovil commented Nov 28, 2022

@spiffmon thanks for the notes. I split out the addition of CLI11 to #2107 (so this PR depends on that one)

With regards to the use of headers here, the reason I'd done that was certain tools like usdedit depended on calling usdcat, so I figured that instead of calling the executable, it could have just included the symbols. However looking at it closer, it is just converting the single layer to usda, which is (IMHO) better suited to just being inlined.

So I've removed all the headers I had, and now usdcat is just a single cpp file, plus the necessary changes to the CMakeLists. I've removed the usdcat.py file as well in the latest changes

@pixar-oss pixar-oss merged commit eee14ca into PixarAnimationStudios:dev Dec 10, 2022
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.

4 participants