Horrendous Rust code organization
As I mentioned in issue #188 (closed); it was a nightmare to bisect an issue between 2.41.0 and 2.41.1.
The minimum best practice when making a commit is that:
- The code compiles
- The code runs
- The code runs correctly
Most of the commits fail not just with one issue but multiple ones.
This a non-exhaustive list of the kinds of issues I found:
- Missing dependency commits (cairo#c86b8d01)
- Wrong Rust dependencies
- Missing files (rust/build.rs)
- Unfinished code (rsvg_node_marker_new)
- Missing sources from the build system
- Local home repositories (file:///home/federico/)
Additionally, while testing multiple commits, it's useful that the compilation happens fast, therefore it makes sense to not compile all the dependencies. Rust seems to have useful features to this end, but they are poorly used in librsvg.
If you are not going to be using a crate, then at the very least the dependency should point to a static object: a tag, a commit, or at the very least a static branch. It should not be pointing to a development or master branch, because then the Cargo.lock file is going to contain many revisions, which means other people trying to build the project are going to constantly have to recompile the dependencies as well.
The dependency should have been:
[dependencies.cairo-rs]
git = 'https://github.com/federicomenaquintero/cairo.git'
branch = 'matrix_try_invert'
But that's not all. Whatever fixes are on the 'matrix_try_invert' branch should eventually go on a release, and then to a publish crate, and once they do, there's no need to clone that code to compile that particular commit.
That's why Rust provides the patch section, so the code compiles correctly even if the new code is still not in crates-io:
[dependencies]
cairo-rs = "0.1.4"
[patch.crates-io]
cairo-rs = { git = 'https://github.com/federicomenaquintero/cairo.git', branch = 'matrix_try_invert' }
This code would work even though cairo-rs 0.1.4 is still not released, and when it is released, the patch section will be ignored.
What actually happened is that librsvg was tracking the master branch of many repositories, and thus the commit in Cargo.lock was jumping all over the place. Eventually it landed on commit c86b8d01 which contained the 'matrix-try-invert' fix.
Probably due to some equally poor Git practices, somebody rebased the 'master' branch, or pushed forced something, so the commit c86b8d01 is now dangling, and can't be fetched any more, and thus, after commit c86b8d01 is needed, the build is broken.
The disappearance of commit c86b8d01 wouldn't have been a problem if somebody had bothered to use the patch section properly and pushed a crate with the fix. But that's not what happened.
Not only was the branch not used, nor the patch section not used, nor the commit c86b8d01 saved, but the release 0.1.4 was never made. So the only reliable way to make the code compile is to fetch 0.1.3 and backport the relevant commits.
Moreover, releases are not tagged, so I had to use some 'git cat-file' tricks in order to figure out which commits corresponded to which crate-io versions.
Additionally, developers shouldn't put their home directories in Cargo.toml, that's what the paths section on the config file is for:
~/.cargo/config
paths = ["/home/felipec/src/nom"]
I had to basically rewrite the whole history of 2.41.0..2.41.1 to make the code somewhat compilable. Only then, and after chasing the various bugs I listed before was I able to track the issue.
This is unacceptable. It should be the job of the maintainer to make sure the code compiles, not the code clients (package maintainers, developers, users).
Even if all the build issues were fixed, it's clear Rust is not yet a stable language, nor are the Rust dependencies of librsvg. And it's also clear nobody has really analyzed the performance of the Rust implementation, nor the functional changes.
If it was up to me, I would revert back, remove all the Rust code and start from scratch. Add the code again patch by patch, but make the Rust implementation OPTIONAL. That's the only reliable way to ensure there will be no regressions.
Rule no. 1 of a library is: ensure it keeps working the same way as before. Right now librsvg is completely broken.
I'll be staying in 2.40.20.