Merge pull request #1462 from google/maintenance-notes

Maintenance documentation part one.

book/src/SUMMARY.md

@@ -18,6 +18,7 @@
 - [Large codebases](large_codebase.md)
 - [Examples](examples.md)
 - [Credits](credits.md)
+- [Reporting bugs](reporting_bugs.md)
 - [Contributing](contributing.md)
 - [Code of Conduct](code-of-conduct.md)
 

book/src/contributing.md

@@ -53,8 +53,8 @@ to `cxx`.
 However, most of the actual code is in `engine/src/conversion/mod.rs`.
 
 At the moment we're using a slightly branched version of `bindgen` called `autocxx-bindgen`.
-It's hoped this is temporary; some of our changes are sufficiently weird that it would be
-presumptious to try to get them accepted upstream until we're sure `autocxx` has roughly the right approach.
+It's hoped this is temporary (see [here](https://github.com/google/autocxx/issues/124)
+for status.)
 
 ## How to develop
 
@@ -79,40 +79,7 @@ parameterize the test suite to do both.)
 
 ## Reporting bugs
 
-If you've found a problem, and you're reading this, *thank you*! Your diligence
-in reporting the bug is much appreciated and will make `autocxx` better. In
-order of preference here's how we would like to hear about your problem:
-
-* Raise a pull request adding a new failing integration test to
-  [`integration_test.rs`](https://github.com/google/autocxx/blob/main/integration-tests/tests/integration_test.rs)
-* Minimize the test using `tools/reduce`, something like this:
-  `target/debug/autocxx-reduce file -d "safety!(unsafe_ffi)" -d
-  'generate_pod!("A")' -I ~/my-include-dir -h my-header.h -p
-  problem-error-message -- --remove-pass pass_line_markers`
-  This is a wrapper for the amazing `creduce` which will take thousands of lines
-  of C++, preprocess it, and then identify the minimum required lines to
-  reproduce the same problem.
-* Use the C++ preprocessor to give a single complete C++ file which demonstrates
-  the problem, along with the `include_cpp!` directive you use.
-  Alternatively, run your build using `AUTOCXX_REPRO_CASE=repro.json` which should
-  put everything we need into `output.h`. If necessary, you can use the `CLANG_PATH`
-  or `CXX` environment variables to specify the path to the Clang compiler to use.
-* Failing all else, build using
-  `cargo clean -p <your package name> && RUST_LOG=autocxx_engine=info cargo build -vvv`
-  and send the _entire_ log to us. This will include two key bits of logging:
-  the C++ bindings as distilled by `bindgen`, and then the version which
-  we've converted and moulded to be suitable for use by `cxx`.
-
-## Bugs related to linking problems
-
-Unfortunately, _linking_ C++ binaries is a complex area subject in itself, and
-we won't be able to debug your linking issues by means of an autocxx bug report.
-Assuming you're using autocxx's build.rs support, the actual C++ build and
-managed by the [`cc`](https://crates.io/crates/cc) crate. You can find
-many of its options on its [`Build` type](https://docs.rs/cc/latest/cc/struct.Build.html).
-If you need to bring in an external library, you may also need to emit certain
-[print statements from your `build.rs`](https://doc.rust-lang.org/cargo/reference/build-scripts.html#rustc-link-lib)
-to instruct cargo to link against that library.
+Moved to [its own document](reporting_bugs.md).
 
 ## How to contribute to this manual
 
@@ -121,3 +88,164 @@ More examples in this manual are _very_ welcome!
 Because `autocxx` examples require both Rust and C++ code to be linked together,
 a custom preprocessor is used for this manual. See one of the existing examples
 such as in `index.md` to see how to do this.
+
+## Maintenance responsibilities
+
+autocxx is currently in maintenance mode. In future, it may undergo further
+feature development, but for now the job is just to keep it stable
+and functional.
+
+Events may occur which nonetheless require changes:
+
+* Changes to Rust
+* Changes to `bindgen`
+* Pull requests raised against autocxx
+* Issues reported against autocxx
+
+Here's what to do. If issues are reported, encourage the reporter
+to raise a PR with a minimized test case by pointing them at the
+["reporting bugs"](reporting_bugs.md) page. This resolves all concerns about
+reproducibility. autocxx is quite sensitive to the environment in which it
+runs, e.g. standard library header files in use, and it's rare to be able
+to reproduce a reporter's bug without them raising a reproducible test case
+like this.
+
+If CI starts to fail due to a Rust change (or,
+if we [manage to unfork bindgen, a bindgen change](https://github.com/google/autocxx/issues/124))
+then raise a pull request to fix it. (Most commonly CI starts to fail
+because of more aggressive clippy lints).
+
+For user-contributed pull requests, merge them if you can! For reproducible
+bug reports, try to investigate them. A fair proportion of the bug reports
+boil down to certain key known areas of technical debt or limitations,
+described below, and you can mark them as a duplicate or similar.
+
+autocxx tends to make a release every month or two, dependent on what changes
+have been made. Ideally, autocxx would release after every single pull
+request but the process takes about 15 minutes (see below) so it's not
+that frequent.
+
+## Release process
+
+To make a new release of autocxx,
+
+* First ensure there's green CI on github.
+* Check out `main` locally and ensure it's up to date with `origin/main`.
+* Make a new branch
+* Run `tools/upgrade-version.sh OLD NEW` where `OLD` is the previous
+  released version number, and `NEW` is the new version number.
+* Commit that and make a PR; ensure it passes tests on github CI.
+* If so, merge that PR, and update locally.
+* Run `tools/publish-all.sh`. This will do the actual `cargo publish`
+  for all the various crates.
+* On [github releases](https://github.com/google/autocxx/releases),
+  choose Draft a new release. Add a tag for `v0.X.Y`. Go through
+  the process to automatically create release notes.
+
+## Rolling bindgen
+
+autocxx currently depends upon a fork of bindgen called `autocxx-bindgen`.
+[This issue](https://github.com/google/autocxx/issues/124) is an attempt
+at entirely unforking bindgen. There are about 14 upstream PRs required;
+we should work hard to make required changes to get them accepted,
+because then this section becomes irrelevant and you'll never need to follow
+these steps.
+
+Otherwise, periodically, you'll need to merge bindgen into autocxx-bindgen.
+[`autocxx-bindgen` is kept in this repository](https://github.com/adetaylor/rust-bindgen)
+in the `master` branch (which should be renamed eventually if we don't
+unfork bindgen).
+
+To update it,
+
+* Check out the master branch of that repo
+* Make a new branch
+* `git pull <upstream repo> main` (note that we use merge rather than
+  rebase)
+* Resolve conflicts and commit.
+* Push this branch to [the autocxx-bindgen repo](https://github.com/adetaylor/rust-bindgen)
+* Make a new branch of autocxx
+* Amend [`engine/Cargo.toml`](https://github.com/google/autocxx/blob/main/engine/Cargo.toml#L34)
+  to point to the new git branch of autocxx-bindgen instead of using
+  a published version (see the commented-out line)
+* Maybe `cd integration-tests; cargo test` to ensure things build and seem to
+  work... but you won't really know until you push to github CI
+* So, push your autocxx and make a pull request
+* If everything passes on CI, bump the `autocxx-bindgen` version number.
+  This needs to be done in `bindgen/Cargo.toml` and `Cargo.toml`. Commit that.
+* Push directly onto the master branch of
+  [the autocxx-bindgen repo](https://github.com/adetaylor/rust-bindgen).
+  You can't raise this roll as a pull request because Google's CLA tooling
+  will reject it.
+* `cargo publish` the new autocxx-bindgen version.
+* Amend your `autocxx` PR to switch to the published `autocxx-bindgen` version,
+  and push that.
+* Ensure CI still passes.
+* If so, merge your `autocxx` PR.
+
+## Major areas of tech debt
+
+The core of autocxx is the `Api` enum. That's solid. It's created by parsing
+bindgen output, and exists all the way through to Rust and C++ codegen.
+It is annotated by various analysis phases; it's parameterized by
+the analysis phase so it's literally impossible to access those annotations
+before they exist.
+
+Because this core is solid, most of the areas of tech debt are discrete and
+don't really overlap with each other. If you wish to tackle them, they
+can be tackled fairly independently.
+
+Without further ado, they are:
+
+1. **Naming**. autocxx deals with lots of names - the original C++ name,
+   the name chosen by bindgen, the name we want to give to cxx, etc.
+   When one kind of name is used for another purpose, certain invariants
+   need to be applied, e.g. avoiding conflicting overloads, or avoiding
+   built-in cxx names such as `UniquePtr`. So, these different kinds of
+   names really need to be encapsulated in newtype wrappers which
+   enforce these invariants. This has been [started here](https://github.com/google/autocxx/issues/520).
+   The other tricky aspect here is that we deal with name conflicts in
+   two different ways. What are name conflicts? bindgen gives us
+   a hierarchical namespace which we have to map to a flat namespace to give
+   to cxx. For types, we just reject any duplicate names. For funtions, though,
+   we go to some efforts to rename them to be non-conflicting. We should
+   become uniform here by abstracting the name deconfliction stuff from the
+   function analysis.
+
+2. **Fork of bindgen**. As noted under [rolling bindgen, above](#rolling-bindgen),
+   we currently use a fork of bindgen called `autocxx-bindgen`. This carries
+   a maintenance burden as we roll upstream. Also as noted above, we're
+   making good progress on undoing this. Tracked
+   [here](https://github.com/google/autocxx/issues/124).
+
+3. **Sensitivity to bindgen bugs caused by constructor calculations**.
+   `bindgen` isn't perfect. When it mis-generates a type, users can
+   ask bindgen to mark that type as "opaque", in which case it's represented
+   as just an array of bytes. This is especially useful for complex templated
+   C++ types where bindgen still has a number of bugs. Unfortunately, users
+   of autocxx don't get that luxury. In order to calculate whether types
+   are movable, autocxx requires visibility into all the fields.
+   That means that any types marked opaque by bindgen are pretty useless,
+   and we never currently ask bindgen for opaque types.
+   `bindgen` might be in a better place to request implicit constructors,
+   then we can solve these problems. This is tracked
+   [here](https://github.com/google/autocxx/issues/1461).
+
+4. **`CppRef` and friends.** `cxx` (and therefore `autocxx`) can be used
+   to create multiple mutable references to the same Rust memory.
+   This is UB. In cxx this problem applies only to types marked `Trivial`
+   since others are zero-sized. In `autocxx`, we give all types a size
+   so they can be allocated on the stack, and so the problem is worse.
+   (This is explained more
+   [here](https://github.com/google/autocxx/blob/main/engine/src/conversion/codegen_rs/non_pod_struct.rs#L41)).
+   The intended solution here is to cease to use Rust references to
+   represent C++ references - instead using a newtype wrapper called
+   `CppRef<T>`. `autocxx` can already work in this mode using
+   [`unsafe_references_wrapped`](https://docs.rs/autocxx/latest/autocxx/macro.safety.html)
+   but for now it works well only on nightly Rust, pending the merge of the
+   [arbitrary self types v2 RFC](https://github.com/rust-lang/rust/pull/135881).
+   Even then, this isn't quite perfect because `cxx` expects to use
+   Rust references, and its key types (such as `cxx::UniquePtr`) provide
+   `Deref` implementations which point in that direction. This isn't a
+   huge problem but I mention it here because it's one of the very few
+   occasions in which our dependence on `cxx` is limiting for us.

book/src/reporting_bugs.md

@@ -0,0 +1,61 @@
+
+## Reporting bugs
+
+If you've found a problem, and you're reading this, *thank you*! Your diligence
+in reporting the bug is much appreciated and will make `autocxx` better.
+
+However, we get a lot of incoming bugs, and we are limited by our ability
+to triage them. Please help us by making a good bug report.
+
+Much of this is about minimizing the size of your test case so it does not
+depend on your system's C++ headers or any copyrighted code. To minimize the
+test case, you can use `tools/reduce`, something like this:
+
+`cd tools/reduce; cargo run -- file -d "safety!(unsafe_ffi)" -d
+'generate_pod!("A")' -I ~/my-include-dir -h my-header.h -p
+problem-error-message -- --remove-pass pass_line_markers`
+This is a wrapper for the amazing `creduce` which will take thousands of lines
+of C++, preprocess it, and then identify the minimum required lines to
+reproduce the same problem. This can sometimes take _days_ to run - that's
+normal - if this is a problem, try to minimize the test case manually, or
+do a combination.
+
+Once you have a minimal test case:
+
+* First see if it triggers a problem when you're using pure `bindgen`.
+  autocxx is very dependent on `bindgen` generating correct code.
+  To do this, fetch the [`bindgen` code](https://github.com/rust-lang/rust-bindgen)
+  then run
+  `cargo run -- XYZ.hpp --no-layout-tests --enable-cxx-namespaces --allowlist-type ABC-- -std=c++14`
+  `XYZ.hpp` is your header (the extension is important) and `ABC` is the type
+  you wish to generate. Assuming you minimized your test case in advance,
+  this should generate just a few lines of Rust code and you can inspect it
+  (or even build it) to see if it seems to be problematic.
+* If the problem is in `bindgen`, report it to `bindgen`.
+* If the problem is in `autocxx`, please report a bug to us. In order of preference:
+  * Raise a pull request adding a new minimized failing integration test to
+    [`integration_test.rs`](https://github.com/google/autocxx/blob/main/integration-tests/tests/integration_test.rs)
+  * Simply raise an issue with the minimized code.
+
+If you can't get minimization to work:
+* Use the C++ preprocessor to give a single complete C++ file which demonstrates
+  the problem, along with the `include_cpp!` directive you use.
+  Alternatively, run your build using `AUTOCXX_REPRO_CASE=repro.json` which should
+  put everything we need into `output.h`. If necessary, you can use the `CLANG_PATH`
+  or `CXX` environment variables to specify the path to the Clang compiler to use.
+* Failing all else, build using
+  `cargo clean -p <your package name> && RUST_LOG=autocxx_engine=info cargo build -vvv`
+  and send the _entire_ log to us. This will include two key bits of logging:
+  the C++ bindings as distilled by `bindgen`, and then the version which
+  we've converted and moulded to be suitable for use by `cxx`.
+
+## Bugs related to linking problems
+
+Unfortunately, _linking_ C++ binaries is a complex area subject in itself, and
+we won't be able to debug your linking issues by means of an autocxx bug report.
+Assuming you're using autocxx's build.rs support, the actual C++ build and
+managed by the [`cc`](https://crates.io/crates/cc) crate. You can find
+many of its options on its [`Build` type](https://docs.rs/cc/latest/cc/struct.Build.html).
+If you need to bring in an external library, you may also need to emit certain
+[print statements from your `build.rs`](https://doc.rust-lang.org/cargo/reference/build-scripts.html#rustc-link-lib)
+to instruct cargo to link against that library.