Pull request policies

[diizzyy]

This is mainly concerns members and frequent contributors but can we agree on not to open PRs just for the sake of it?

PRs should be run-tested on at least one platform before being submitted unless maintainer of a package has clearly stated otherwise. Also, commit message should contain all changes being made.
If a change isn't specific to OpenWrt please try to upstream it first.

Given our limited resources I don't think we need more administrative work than needed.

Also while I'm at it, how should we handle new package submissions?
We have quite a few queued, could this be a good idea?
#8509 (comment)

[feckert]

I am new to this but I also think we should vote if we like this new package to get merged or not. Some of this packages have a special use case or an alternative package is already merged.
So the question is do we realy need this and what is the use case?
And if none of the committer are familiar with this new package, and the listed packages maintainer is not there anymore, then the package will on the long run unusable if the upstream package get developed further.
And (this is my opinion) if I do not use this package It is hard for me to evaluate this and so i am not willed (good feeling) to merge this.

[cotequeiroz]

I'm also new to this, and new packages are particularly hard for me to review and judge. Apparently to most reviewers as well, since new packages keep piling up, some of them for well over a year. Some guidelines are definitely needed, and voting will not really fix it. How can you vote if you don't know what to look for?
Take #5160, for example. It was open in 2017, had regular pushes, which for me is an indication of maintenance; it came from a user I had interacted with in the past about node packages. I've compiled, checked its contents and merged it last night. I might have made a mistake, as there was a review past that, but I'm still confident it was the right thing to do.

[diizzyy]

Something along the lines with will this be used by more than a handful of people and does it make sense to have it in. As far as node.js stuff goes it makes more sense to use nxhack's own repo IMHO.

[diizzyy]

@thess @hnyman @neheb @dibdot @champtar @jslachta @BKPepe @mhei @karlp
Any ideas/comments?

[BKPepe]

I can imagine that I would want to add some new package to this repository and people with commit access will be voting about it and I didn't make it just for one vote or they will refuse it all, but if at least one user from the community will find for it usage, so why not add it?

I can see that some packages are not added to OpenWrt like vpn-policy-routing and we should ask people, why they don't want to add it to OpenWrt directly, so all users can benefit from it and not just someone.

It's really bad when PR is stuck for a month even it was reviewed. I also understand why you are asking it. There are a lot of packages, which are not maintained anymore. :/

I have a different point of view of this, but I agree that new packages must be run-tested. About adding changes to upstream, I'm fine with it, but in some situations, it's not possible. E.g. one my PR is blocked, because upstream doesn't respond to my PR, so what's now? The package doesn't work if you didn't compile libcurl yourself.

[diizzyy]

Since we're unable to maintain the current repo in terms of keeping everything up to date new packages needs to have some general interest in order to attract contributors. As far as packages goes, if I were to submit lets say a biological-related package it would make little sense in general to merge such a package or if I wanted to use a version of grep tuned for a very specific use case. Of course there will be software that we're not aware of that's why I suggest a little use case description. I primarily want to avoid package being in limbo state for infinity and heavily dated ones.

Regarding patches the goal is to have as few as possible in the tree. I'm not suggesting that patches will be banned however they should be submitted upstream first unless they're specific for OpenWrt. If upstream is unresponsive it should be noted in the commit message.

[cotequeiroz]

About upstream submissions, I think they don’t need to be accepted; but they are a very good thing. Besides the fact that there might be more people with the same need, and to review it, when accepted, we don’t need to maintain it separately.

[hnyman]

Also while I'm at it, how should we handle new package submissions?
We have quite a few queued, could this be a good idea?

Reference also to the previous discussion about PR policies in 2016-2017:
#3579

Regarding new packages

Like thess says in #3579 (comment) , there should preferably be some indication of a wider interest for a package in the OpenWrt user space, especially if the package does not obviously correspond to a typical home router use case.

Also from resource perspective, if is good to remember that all packages are complied for all ~40 targets and stored for all them. As we are not a general Linux distro with unlimited resources, I don't like merging somebody's personal math conversion library etc. There have also been some package PRs from derivative distros that are apparently built for their end-users, but the usefulness of all of those packages in a wider OpenWrt context is not that obvious.

Regarding merging PRs and testing required

Some thoughts from me:

• version bumps from the package maintainer (and tested by him) should not require much further testing.

  • (Travis was unreliable and had timeouts, but CircleCI seems to be better in compiling pretty much everything. If CircleCI is happy about a version bump, it is usually ok also in the buildbot. But still, also it can create false positives and complain unnecessarily)

• Fixes for packages that currently fail in buildbot are also something where lighter testing can be reasonable, as the package is already broken.

• It is rather informative to look at various targets at https://downloads.openwrt.org/snapshots/faillogs/ , especially after PRs have been merged. There are several packages that have been broken for quite some time. (Currently the list is rather short, as some people like @neheb made a major effort a few months ago to fix a bunch packages. Not all of those fixes went ok at the first try, but buildbot is there just for testing)

• Buildbot environment is different than a personal buildroot. As buildbot compiles all kernel modules and all packages, it is quite typical that a package's configure script finds new libraries and activates their usage in the package, causing a dependency error. This is hard to test in personal buildroot, as usually it makes no sense to compile all packages. So, the error materialises in buildbot after the PR has been merged. This easily leads into further PRs fixing the dependency error by disabling/patching away the functionality in the package config, or by adding the missing new dependency (if the functionality is really necessary for the package).

• There are also differences between targets. Some problems materialise only for some targets, so even a normal testing by the maintainer for one target may not reveal the compiled problems for other targets

• some of the new packages for master have been tested by the author only with 18.06, which is a bad practice.

• Cautiousness is needed if any recently added packages are considered to be backported into the 18.06 branch. Due to the dependencies, it is rather easy to break some other already existing packages there. So, usually the new packages should not be backported into the stable branches, especially if the package is a library that other packages can detect and link into in the compile phase.

[cotequeiroz]

I think we should have a criteria to close the PRs as well, especially for new packages. If they do not meet our criteria to be merged, I think it is fair to leave a note explaining why they are not being accepted, and close them. We shouldn't just keep them in a limbo. They can always be reopened/recreated if circumstances change.