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

Improve the developer quickstart documentation #1144

Conversation

JeanChristopheMorinPerso
Copy link
Member

As note in https://academysoftwarefdn.slack.com/archives/CMQ9J4BQC/p1636760978147000, the developer quickstart documentation is out of date right now.

This PR removes obsolete instructions, adds some more instructions and also use modern methodologies (e.g. call python -m pip install of pip, don't call python setup.py, etc).

Hopefully it should be clear enough and the additions I made will help future contributors.

@JeanChristopheMorinPerso
Copy link
Member Author

@ThomasWilshaw Do you think the proposed changed would have made your experience better?

Copy link
Collaborator

@meshula meshula 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 update! I've noted a couple of issues.

docs/tutorials/quickstart.md Outdated Show resolved Hide resolved
docs/tutorials/quickstart.md Outdated Show resolved Hide resolved
@ThomasWilshaw
Copy link
Contributor

@ThomasWilshaw Do you think the proposed changed would have made your experience better?

Yes that's definitely clearer but there's still a few things I don't follow.

  1. In the section "To build OTIO for Python development", where is the given command meant to be run from? The source directory or somewhere else? Also is a note of what the --user option does necessary or will most people know what it's for already?

  2. In the section: "To build OTIO for both C++ and Python development" it implies that you should run setup.py to create all the build files and compile OTIO, is that what we have just done in the previous step when we ran python -m pip install . --user? It also says using setup.py is highly discouraged, despite just saying to run it.

Am I right in thinking that if I were to edit the code base in a way that affected both the Python and C++ sections, the best way for me to compile everything and run the tests would be to run python -m pip install. --user whereas if I were just modifying the C++ code I should use the standard cmake --build... command?

I should just clarify I'm not a very experienced dev (and I particularly don't like actually compiling software ;)) so I could just be misunderstanding things or generally no know what I'm doing :)

@JeanChristopheMorinPerso
Copy link
Member Author

@ThomasWilshaw I just pushed an update which hopefully should answer your questions. Let me know if it's still unclear.

@meshula
Copy link
Collaborator

meshula commented Nov 20, 2021

One thing we haven't gotten to is a comprehensive test suite at the C++ level. We are relying on the python bindings to expose problems. If you are developing in the C++ code, you should do a pip install and run the python tests before submitting a PR. It's up to you whether you also do a pip install while you are iterating the C++ code.

New C++ PRs are expected to have a C++ based test to go along with them. We will need to add a comprehensive test suite at the C++ level.

@ThomasWilshaw
Copy link
Contributor

@ThomasWilshaw I just pushed an update which hopefully should answer your questions. Let me know if it's still unclear.

That's defintely a lot clearer thanks.

@meshula meshula merged commit 3623455 into AcademySoftwareFoundation:main Nov 24, 2021
@JeanChristopheMorinPerso JeanChristopheMorinPerso deleted the improve_developer_doc branch November 28, 2021 15:45
jminor pushed a commit that referenced this pull request Feb 11, 2022
* Improve the developer quickstart documentation and remove obsolete instructions.

* * Clarification of the different C++ code layers in the repo
* Remove usage of --user flag
@ssteinbach ssteinbach added this to the Public Beta 15 milestone Sep 19, 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