Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 517416 - =sys-apps/portage-2.2.10: repoman warning for 'whitespace' is reporting wrong line numbers
Summary: =sys-apps/portage-2.2.10: repoman warning for 'whitespace' is reporting wrong...
Status: CONFIRMED
Alias: None
Product: Portage Development
Classification: Unclassified
Component: Repoman (show other bugs)
Hardware: All Linux
: Normal normal (vote)
Assignee: Portage team
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-18 14:34 UTC by Samuli Suominen (RETIRED)
Modified: 2014-07-19 21:22 UTC (History)
1 user (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
whitespace on line 50, repoman says 42 (mdadm-3.3.1-r2.ebuild,2.96 KB, text/plain)
2014-07-18 14:34 UTC, Samuli Suominen (RETIRED)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Samuli Suominen (RETIRED) gentoo-dev 2014-07-18 14:34:55 UTC
Created attachment 380964 [details]
whitespace on line 50, repoman says 42

RepoMan scours the neighborhood...
  ebuild.minorsyn               1
   sys-fs/mdadm/mdadm-3.3.1-r2.ebuild: Trailing whitespace error on line: 42

But in fact the whitespace is in line 50. Repoman got confused by \ char(s).

Attaching the ebuild here as well.
Comment 1 Alexander Berntsen (RETIRED) gentoo-dev 2014-07-18 14:58:06 UTC
It's been this way for a long time -- maybe always. The reason should be obvious. I'm surprised it's not been reported years ago.

Not sure if it's worth fixing with regards to effort v. pay off. Nonetheless, I will take a stab at it during the weekend.
Comment 2 Samuli Suominen (RETIRED) gentoo-dev 2014-07-18 15:06:44 UTC
Can we change the message at least, then?

Trailing whitespace error on line: 42

to

Trailing whitespace error on line (or after the line with \ characters involved): 42
Comment 3 Alexander Berntsen (RETIRED) gentoo-dev 2014-07-19 10:58:02 UTC
I can do that if I end up giving up on actually fixing it. :-P
Comment 4 Alexander Berntsen (RETIRED) gentoo-dev 2014-07-19 13:45:16 UTC
Turns out changing the message is not a good idea, because then we have to do it with every single line-referencing message. For an example of what I am talking about, try e.g. removing quotes from one of your "${FILEDIR}"s.

It would be better to update the man page to reflect that "whooops, repoman gets confused with continuation lines".

Better still would be actually fixing the issue. There is some work going on to refactor repoman. Can someone from that effort please issue a statement here that clarifies whether this is actually worth fixing?
Comment 5 Tom Wijsman (TomWij) (RETIRED) gentoo-dev 2014-07-19 18:24:30 UTC
(In reply to Alexander Berntsen from comment #4)
> Turns out changing the message is not a good idea, because then we have to
> do it with every single line-referencing message. For an example of what I
> am talking about, try e.g. removing quotes from one of your "${FILEDIR}"s.

+1 Line based checks aren't uncommon, we need to keep scalability in mind too.

> It would be better to update the man page to reflect that "whooops, repoman
> gets confused with continuation lines".

-1 Rather define what a line is if we happen to not change output; but really, it would be nice to change output to be more clear to avoid it in the future.

> Better still would be actually fixing the issue. There is some work going on
> to refactor repoman. Can someone from that effort please issue a statement
> here that clarifies whether this is actually worth fixing?

If we refactor with this in mind, it shouldn't be too hard; we could then define the line related errors to all use the same 'error creation' function, which then uses a 'line determination' function to figure out the lines.

Though, the idea as proposed is much easier said than done; because the 'line determination' function is unaware of the check implementation, thus for the best of both worlds I think we can do "in lines 42-50" instead of "on line 50".

If we really want the latter, we will need to change all checks to report the actual line to the appropriate function; this implies some extra duplication and/or effort[1], as well as leaves more room for error when adding checks.

Determining the last line when there is line continuation seems to be trivial to me; it boils down to finding the next line which does not end in a \ followed by zero or more spaces and an EOL, starting with and including the current line.

 [1]: Implementation of this may even differ based on the check at hand.
Comment 6 Alexander Berntsen (RETIRED) gentoo-dev 2014-07-19 21:22:26 UTC
(In reply to Tom Wijsman (TomWij) from comment #5)
> -1 Rather define what a line is if we happen to not change output; but
> really, it would be nice to change output to be more clear to avoid it in
> the future.
I don't really understand what you mean, but I committed a man page note.

> If we refactor with this in mind, it shouldn't be too hard; we could then
> define the line related errors to all use the same 'error creation'
> function, which then uses a 'line determination' function to figure out the
> lines.
> 
> Though, the idea as proposed is much easier said than done; because the
> 'line determination' function is unaware of the check implementation, thus
> for the best of both worlds I think we can do "in lines 42-50" instead of
> "on line 50".
> 
> If we really want the latter, we will need to change all checks to report
> the actual line to the appropriate function; this implies some extra
> duplication and/or effort[1], as well as leaves more room for error when
> adding checks.
> 
> Determining the last line when there is line continuation seems to be
> trivial to me; it boils down to finding the next line which does not end in
> a \ followed by zero or more spaces and an EOL, starting with and including
> the current line.
> 
>  [1]: Implementation of this may even differ based on the check at hand.
I basically don't understand anything you are saying. The few things I am able to discern here sound wrong, from my knowledge of repoman.

But that might be my fault. So I hope you manage to fix it in some way or other in your refactoring branch.