Bug 10346

There's nothing really to design here.

The code that configured a default user (if one wasn't
already configured) was dropped on purpose. We gave everyone
6 years to get their configurations properly set up, and
hence I decided to drop the code.

I can restore that code, but having it as an error works 
towards encouraging a proper workflow, and properly attributing
patches. Anyone that is patching the kernel, should be doing
so as a known user, with properly formatted and signed-off-by
patches.

This bug stays with me.

There is a bigger issue here than just the kernel tools. buildhistory runs into this, as does eSDK and with the increasing use of containers, more people are running into this problem.

So I suspect we need:
a) a higher level sanity test which the user hits up front telling them there are problems, not having the build break after running for 30mins
b) some standardised way of handling this at the project level

The reason it was "taken" from Bruce was that several people said the above was "not a Bruce type issue" in the triage meeting. Is fixing this in a wider scope than just the kernel tools something you plan to do or not?

(In reply to comment #3)
> There is a bigger issue here than just the kernel tools. buildhistory runs
> into this, as does eSDK and with the increasing use of containers, more
> people are running into this problem.
> 
> So I suspect we need:
> a) a higher level sanity test which the user hits up front telling them
> there are problems, not having the build break after running for 30mins
> b) some standardised way of handling this at the project level
> 
> The reason it was "taken" from Bruce was that several people said the above
> was "not a Bruce type issue" in the triage meeting. Is fixing this in a
> wider scope than just the kernel tools something you plan to do or not?

I had planned on doing a sanity check, but yes, if this is also wanted
for more than just the kernel, and it is wanted for the release, someone
else should grab this, since I'll be focused on completing the 4.8
kernel.

I can make a bug for the kern-tools only and restore that code if there's
value in that before the release as well.

(In reply to comment #1)

> I can restore that code, but having it as an error works 
> towards encouraging a proper workflow, and properly attributing
> patches. Anyone that is patching the kernel, should be doing
> so as a known user, with properly formatted and signed-off-by
> patches.

FWIW, this bug is not asking to break the proper workflow. Properly authored/formatted patches we can still require.

This bug is about not being able to apply those properly formatted patches if the committer information is missing.

(In reply to comment #5)
> (In reply to comment #1)
> 
> > I can restore that code, but having it as an error works 
> > towards encouraging a proper workflow, and properly attributing
> > patches. Anyone that is patching the kernel, should be doing
> > so as a known user, with properly formatted and signed-off-by
> > patches.
> 
> FWIW, this bug is not asking to break the proper workflow. Properly
> authored/formatted patches we can still require.
> 
> This bug is about not being able to apply those properly formatted patches
> if the committer information is missing.

Someone needs to change the title on this.

Whatever the general fix is for this, it doesn't need to touch the kernel tools at all.

Either it is transparent to them, or I'll make the change, since I've always had code to do this (and dropped it on purpose), there's nothing that needs to be done in that area.

I spoke with Richard regarding this and I'm going to create a shared function which checks for a local git config user and email, and if no values are found, sets them with some default values.  See buildhistory.bbclass:

if ! git config user.email > /dev/null ; then
        git config --local user.email "buildhistory@${DISTRO}"
if ! git config user.name > /dev/null ; then
        git config --local user.name "buildhistory"

I think that while the sanity check is a viable solution, it is a very broad stroke, and hopefully a shared function will be less intrusive.

Thoughts?

(In reply to comment #7)
> I spoke with Richard regarding this and I'm going to create a shared
> function which checks for a local git config user and email, and if no
> values are found, sets them with some default values.  See
> buildhistory.bbclass:
> 
> if ! git config user.email > /dev/null ; then
>         git config --local user.email "buildhistory@${DISTRO}"
> if ! git config user.name > /dev/null ; then
>         git config --local user.name "buildhistory"
> 
> I think that while the sanity check is a viable solution, it is a very broad
> stroke, and hopefully a shared function will be less intrusive.
> 
> Thoughts?

As long as the change isn't in the kernel tools, or bbclasses, it looks fine to me. Since it will transparently be used by anyone's routine that is using git to apply changes.

> As long as the change isn't in the kernel tools, or bbclasses, it looks fine
> to me. Since it will transparently be used by anyone's routine that is using
> git to apply changes.

I was thinking of putting it in oe/utils, and then finding all the places where we might call git and prefacing it a call to this function. This means that while the kernel tools won't need to change, any build functionality that utilizes those tools would need to preface their call with a call to this function. Essentially any built-time functionality that calls git would need to have a call to this "user/email check". In typing that I realize this will be a tricky undertaking, and certainly hard to test.

Since the code freeze is Monday perhaps we could implement the broadsword sanity check for now, and think about a more fine grained solution for the future.

Stay sane for now, debate better changes for the future?

(In reply to comment #9)
> > As long as the change isn't in the kernel tools, or bbclasses, it looks fine
> > to me. Since it will transparently be used by anyone's routine that is using
> > git to apply changes.
> 
> I was thinking of putting it in oe/utils, and then finding all the places
> where we might call git and prefacing it a call to this function. This means
> that while the kernel tools won't need to change, any build functionality
> that utilizes those tools would need to preface their call with a call to
> this function. Essentially any built-time functionality that calls git would
> need to have a call to this "user/email check". In typing that I realize
> this will be a tricky undertaking, and certainly hard to test.
> 
> Since the code freeze is Monday perhaps we could implement the broadsword
> sanity check for now, and think about a more fine grained solution for the
> future.
> 
> Stay sane for now, debate better changes for the future?

The broadsword sanity check worries me a bit as there are many cases where someone doesn't need to set this at all. Am I right in thinking that standard linux-yocto doesn't break if the git config isn't set? What are the circumstances where it will error? Certain kinds of patches in SRC_URI?

The trouble is we shouldn't show errors to the user about things which don't matter, we should only really show errors when they really do matter, but that means known in advance when errors will occur. Can we tell?

I'm going to look into how we use kernel-tools and see if I can't check / fix the issue transparently (without changing the tools).

(In reply to comment #11)
> I'm going to look into how we use kernel-tools and see if I can't check /
> fix the issue transparently (without changing the tools).

If the plan is to do this only for the kernel-tools, give me the bug, I'll restore my old code and fix this in 5 minutes.

> If the plan is to do this only for the kernel-tools, give me the bug, I'll
> restore my old code and fix this in 5 minutes.

I'm going to see if I can abstract out the buildtools / eSDK stuff as well.

> As long as the change isn't in the kernel tools, or bbclasses, it looks fine
> to me. Since it will transparently be used by anyone's routine that is using
> git to apply changes.

Looks like I would have to change the kernel-yocto.bbclass file in order to import / run the code that does the check & fix. I'd import the same code for buildhistory.bbclass.

I cannot see a way to do this completely transparently to the bbclass(es), though I may be myopic from lack of experience here. Any thoughts?

(In reply to comment #14)
> > As long as the change isn't in the kernel tools, or bbclasses, it looks fine
> > to me. Since it will transparently be used by anyone's routine that is using
> > git to apply changes.
> 
> Looks like I would have to change the kernel-yocto.bbclass file in order to
> import / run the code that does the check & fix. I'd import the same code
> for buildhistory.bbclass.
> 
> I cannot see a way to do this completely transparently to the bbclass(es),
> though I may be myopic from lack of experience here. Any thoughts?

I'm not sure how to make it completely transparent either, but I'm
ok with a change to the kernel-yocto.bbclass (or any other bbclass)
on a case by case basis.

My objection is only if the kern-tools themselves need to be changed,
since I can do that change myself.

> I'm not sure how to make it completely transparent either, but I'm
> ok with a change to the kernel-yocto.bbclass (or any other bbclass)
> on a case by case basis.
> 
> My objection is only if the kern-tools themselves need to be changed,
> since I can do that change myself.

Understood. I shouldn't need to touch the tools at all, and really shouldn't touch those, since I just learned that they existed this morning :).

Thanks for your help and advice.

I wasn't aware this had come up on the kernel side - but I'm slightly perturbed that we're pulling in different directions given that we've only just gone in the other direction recently elsewhere in the code:

http://git.yoctoproject.org/cgit/cgit.cgi/poky/commit/?id=4b4387455c62cf19fa3d215a11b5d0b1211d4570

The logic is that when an automated script which is mostly not under the user's control applies the patch, it makes less sense to be user's name/email and thus claiming that the user actually made the commit.

See also bug 8703 for what prompted that change.

(In reply to comment #17)
> I wasn't aware this had come up on the kernel side - but I'm slightly
> perturbed that we're pulling in different directions given that we've only
> just gone in the other direction recently elsewhere in the code:
> 
> http://git.yoctoproject.org/cgit/cgit.cgi/poky/commit/
> ?id=4b4387455c62cf19fa3d215a11b5d0b1211d4570
> 
> The logic is that when an automated script which is mostly not under the
> user's control applies the patch, it makes less sense to be user's
> name/email and thus claiming that the user actually made the commit.
> 
> See also bug 8703 for what prompted that change.

For the kernel, we are talking about code that has been around for years now.
If there's an issue with this fix, I'll simply restore what I've always had
for the kernel tools.

This was supposed to be a simple restoration of that functionality, not 
something more complicated.

I've submitted a fix for this and pinged the list, as I haven't heard back and I don't see the fix in master-next.

http://git.yoctoproject.org/cgit.cgi/poky/commit/?id=ef7828c9f2955dfde09a0c83694c8a67cdc34b40