Skip to content

Enhanced cvdr.md documentation#483

Merged
jemoreira merged 10 commits intogoogle:mainfrom
IndiTheo:main
Apr 23, 2026
Merged

Enhanced cvdr.md documentation#483
jemoreira merged 10 commits intogoogle:mainfrom
IndiTheo:main

Conversation

@IndiTheo
Copy link
Copy Markdown
Contributor

@IndiTheo IndiTheo commented Oct 13, 2025

Enhanced cvdr documentation, made it more ad-hoc to newcomers:

  • Added common use case - local AOSP build
  • Clear separation between Debian and non-Debian cases
  • General flow refactor

As a newcomer, it was pretty difficult to get along with the command without reading the code. Opened this PR to make it easier for other people.

@google-cla
Copy link
Copy Markdown

google-cla Bot commented Oct 13, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@IndiTheo IndiTheo marked this pull request as ready for review October 13, 2025 22:09
Copy link
Copy Markdown
Member

@jemoreira jemoreira left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! The documentation is certainly more useful now.

I left some minor comments, we can merge the PR once you have addressed them and signed the Google CLA (see comment from the review bot).

Comment thread docs/cvdr.md Outdated


```bash
CVDR_USER_CONFIG_PATH=/path/to/cvdr.toml ./cvdr --help
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should also make cvdr try to read config files from the default locations when the variables are not defined.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is it the current behavior I can document, or is it on a TODO list?

Copy link
Copy Markdown
Member

@jemoreira jemoreira Apr 20, 2026

Choose a reason for hiding this comment

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

My original comment wasn't relevant at all, sorry about that. Let me try again:

These example cvdr invocations should apply to anyone who built cvdr from source or installed it via debian package. When cvdr is installed in your system the CVDR_USER_CONFIG_PATH is not necessary and including it here and in the other examples could lead to confusion. Instead, add a step to the instructions on how to build it from source to create the following alias:

alias cvdr="CVDR_USER_CONFIG_PATH=/path/to/cvdr.toml $(realpath ./cvdr)"

That way all the examples using just cvdr work for all the ways in which cvdr could have been installed.

For context: When cvdr is installed via debian package it's not copied directly to /usr/bin/cvdr, but somewhere else not in the $PATH. /usr/bin/cvdr is just a shell script that defines environments variables (like config path and http proxies) and does some checks before executing the "real" cvdr binary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see now, thanks.
I addressed it in the docs, but I wonder why not define the default config file paths in code rather than a bash wrapper, so that both the package and the manual builds can read them by default.

Comment thread docs/cvdr.md Outdated
Comment thread docs/cvdr.md Outdated
Comment thread docs/cvdr.md Outdated
Added expected output for successful build process.
@rmuthiah rmuthiah removed their request for review April 15, 2026 17:47
@IndiTheo
Copy link
Copy Markdown
Contributor Author

IndiTheo commented Apr 18, 2026

Thank you for review!

Addressed your comments.

Did the CLA agreement (though not very happy about it).

@IndiTheo IndiTheo requested a review from jemoreira April 18, 2026 16:07
@jemoreira
Copy link
Copy Markdown
Member

Please use a rebase instead of a merge when updating the branch. You can fix the existing merge commit when you squash the extra commits after the PR is approved.

@IndiTheo
Copy link
Copy Markdown
Contributor Author

Please use a rebase instead of a merge when updating the branch. You can fix the existing merge commit when you squash the extra commits after the PR is approved.

Sure, I can do it. But can't we just rely on automatic squash on merge?

@jemoreira
Copy link
Copy Markdown
Member

Please use a rebase instead of a merge when updating the branch. You can fix the existing merge commit when you squash the extra commits after the PR is approved.

Sure, I can do it. But can't we just rely on automatic squash on merge?

We use a merge queue, which is configured to keep your commits and rebase them on top of main. We preserve individual commits from PRs because that let us revert them individually too.

@jemoreira jemoreira merged commit 63dac54 into google:main Apr 23, 2026
1 check passed
@IndiTheo
Copy link
Copy Markdown
Contributor Author

Please use a rebase instead of a merge when updating the branch. You can fix the existing merge commit when you squash the extra commits after the PR is approved.

Sure, I can do it. But can't we just rely on automatic squash on merge?

We use a merge queue, which is configured to keep your commits and rebase them on top of main. We preserve individual commits from PRs because that let us revert them individually too.

Got it, thank you!

Guess it's less relevant for a single-file docs PR, but I will remember for future contributions.

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