Project:Reviewers/Common issues

This page lists the most common issues that were found in the late commits.

Commits and commit messages

Mixing multiple changes into one commit

Many developers are still doing multiple semi-relevant changes in a single commit. This should generally be avoided to allow easy reverts.

Bad example:

1. X commits app-foo/bar: version bump, remove old and then goes away,
2. QA checks indicate that multiple stable packages required removed old version of app-foo/bar, and are broken now,
3. developers have to manually choose an appropriate version and re-add it,
4. X may have to remove the re-added version and re-add another one.

Good example:

1. X commits app-foo/bar: version bump,
2. X commits app-foo/bar: remove old,
3. QA checks indicated that multiple stable packages are broken,
4. developers quickly revert the second commit and restore integrity,
5. X re-commits the removal, correcting it as necessary.

Generic suggestions when to split and when not to:

• always remove old versions separately.
• If performing ebuild changes that require a revbump and removing the pre-revbump version, both should be done in a single commit. Most importantly, this lets git detect move easily, and show a nice diff.
• If a package has a live ebuild, you can split a version bump into a series of commits applying different changes to the live ebuild, and they a final version bump commit that copies the live ebuild into release.

Commit messages

Some developers are still using CVS-style commit messages which don't fit well in git. In particular this involves over-long commit messages and messages lacking a proper summary line.

A git commit message always has the following form:

CODE Example git commit message

'"`UNIQ--pre-00000000-QINU`"'

The first line is the summary line which is limited to ~75 characters and is the only line shown in the shortlog. It is usually prefixed by a component to which the commit applies to (package, category, eclass...) and it should provide all important details about the commit while fitting in the length. It's fine to use abbreviations or skip less important bug numbers. But you shouldn't skip major changes.

A detailed commit message (optional) starts at the third line. You use it if summary was too short to list all the changes, or if they need an additional explanation. It is usually a good practice to repeat what was said in summary. Also remember to wrap text.

The last 'paragraph' of the commit message is traditionally used for 'tags'. Those are rather free-form, key: value information. They can be used to reference people (Signed-off-by, Reviewed-by, Suggested-by, etc.), URLs (Bug, References, Fixes), commits (Fixes, Reverts).

Example of bad commit message:

CODE Example bad commit message (lacking summary)

'"`UNIQ--pre-00000003-QINU`"'

In this case, shortlog suggests the ebuild only added new version, and the detailed body lacks information about version bump. If you submitted the commit as a patch via git send-email, you'd notice the summary line ends up in mail subject, and the body only contains the remaining text.

The commit message for this commit could be instead:

CODE Example fixed commit message

'"`UNIQ--pre-00000006-QINU`"'

In this case, the expanded body is entirely optional and should be used only if it can provide additional details compared to summary. Tags can be used to provide full 'clickable' bug URL, or mention more bugs while keeping only most relevant one(s) in summary.

Ebuild issues

Missing or incorrect error handling

• missing die on pusdh or popd. These are shell commands (directory stack builtins) and can fail in the same way cd can fail
• missing die on 'cat'-blocks, e.g.

cat > script.sh <<-_EOF_
#!/bin/sh

Shell script content
_EOF_

instead should be

cat > script.sh <<-_EOF_ || die
#!/bin/sh

Shell script content
_EOF_

• echo "bar" > foo/bar can fail, if the directory 'foo' does not exist or if there are permission problems
• touch foo/bar can fail, if the directory 'foo' does not exist or if there are permission problems

Missing or incorrect SLOT-dependency

There are various concepts for designing the SLOT naming scheme for a given package. A few very popular ones are:

• have SLOT=0 be the only SLOT that people can build against, all other SLOTs will be for binary-compatibility only (e.g. media-libs/libpng or dev-libs/openssl)
• have no SLOT=0, but version-specific SLOTs that all can be built against. Usually these packages then need an eclass to allow choosing the right SLOT at build-time and adjusting library and include-dirs (e.g. x11-libs/wxGTK or dev-lang/vala)
• ...more

Whenever you depend on a package, check if it is slotted and figure out what those SLOTs actually mean. pkgcheck throws various warnings, that are often fixed incorrectly, e.g.:

dev-libs/openssl:=
dev-libs/openssl:*

These are both incorrect if you build against openssl, because openssl uses SLOT=0 as the only one you can build against. The following however are correct:

dev-libs/openssl:0=
dev-libs/openssl:0

Depending on whether you want to trigger a rebuild on Subslot changes.

Missing or incorrect USE flags dependencies

Some packages have extra pitfalls when it comes to choose which USE flags must be activated for a given library. Never assume that the user has set any sane configuration.

Popular pitfalls are media-libs/libsdl and media-libs/libsdl2. Most packages compile successfully with libsdl, even if it has all USE flags disabled. Errors will occur during runtime however. It is extremely rare that

RDEPEND="media-libs/libsdl"

is correct. Always try to figure out which SDL features are actually used. Worst case you have to rebuild libsdl with different USE flag configurations and runtime-test your package against them.