| Message ID | 1386198891-17968-2-git-send-email-thomas.petazzoni@free-electrons.com |
|---|---|
| State | Accepted |
| Commit | edeb7d53978f20acc2306b71bc818e81298a68b3 |
| Headers | show |
Thomas, Samuel, All, On 2013-12-05 00:14 +0100, Thomas Petazzoni spake thusly: > From: Samuel Martin <s.martin49@gmail.com> > > This patch fixes an issue that occurs during the manual build process > which will occur when BR2_EXTERNAL is introduced. > > During the package list generation, the python script using kconfiglib > module reads and parses the Config.in files. So, symbols, including > environment variables, got expanded and/or resolved. In > kconfiglib.py, this patch fixes the regex that did not allow to use > numbers in the environment variable names, so '$BR2_EXTERNAL' got > wrongly expanded like it was '${BR}2_EXTERNAL': > > <snip> > >>> Updating the manual lists... > Traceback (most recent call last): > File "/opt/buildroot/master/support/scripts/gen-manual-lists.py", line 375, in <module> > buildroot = Buildroot() > File "/opt/buildroot/master/support/scripts/gen-manual-lists.py", line 216, in __init__ > self.root_config)) > File "/opt/buildroot/master/support/scripts/kconfiglib.py", line 214, in __init__ > self.top_block = self._parse_file(filename, None, None, None) > File "/opt/src/buildroot/master/support/scripts/kconfiglib.py", line 919, in _parse_file > return self._parse_block(line_feeder, None, parent, deps, visible_if_deps, res) > File "/opt/buildroot/master/support/scripts/kconfiglib.py", line 1114, in _parse_block > self.base_dir)) > IOError: /opt/buildroot/master/Config.in:490: sourced file "$BR2_EXTERNAL/Config.in" (expands to > "2_EXTERNAL/Config.in") not found. Perhaps base_dir > (argument to Config.__init__(), currently > "/opt/buildroot/master") is set to the wrong value. > docs/manual/manual.mk:2: recipe for target 'manual-update-lists' failed > make: *** [manual-update-lists] Error 1 > </snip> > > Reported-by: Ryan Barnett <rjbarnet@rockwellcollins.com> > Signed-off-by: Samuel Martin <s.martin49@gmail.com> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr> Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr> Regards, Yann E. MORIN. > --- > support/scripts/kconfiglib.py | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/support/scripts/kconfiglib.py b/support/scripts/kconfiglib.py > index 0704cc0..e40947c 100644 > --- a/support/scripts/kconfiglib.py > +++ b/support/scripts/kconfiglib.py > @@ -2074,7 +2074,7 @@ set_re = re.compile(r"CONFIG_(\w+)=(.*)") > unset_re = re.compile(r"# CONFIG_(\w+) is not set") > > # Regular expression for finding $-references to symbols in strings > -sym_ref_re = re.compile(r"\$[A-Za-z_]+") > +sym_ref_re = re.compile(r"\$[A-Za-z_][0-9A-Za-z_]*") > > # Integers representing symbol types > UNKNOWN, BOOL, TRISTATE, STRING, HEX, INT = range(0, 6) > -- > 1.8.1.2 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Thomas P. Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote on 12/04/2013 05:14:47 PM: > From: Samuel Martin <s.martin49@gmail.com> > > This patch fixes an issue that occurs during the manual build process > which will occur when BR2_EXTERNAL is introduced. > > During the package list generation, the python script using kconfiglib > module reads and parses the Config.in files. So, symbols, including > environment variables, got expanded and/or resolved. In > kconfiglib.py, this patch fixes the regex that did not allow to use > numbers in the environment variable names, so '$BR2_EXTERNAL' got > wrongly expanded like it was '${BR}2_EXTERNAL': > > <snip> > >>> Updating the manual lists... > Traceback (most recent call last): > File "/opt/buildroot/master/support/scripts/gen-manual-lists.py", line 375, in <module> > buildroot = Buildroot() > File "/opt/buildroot/master/support/scripts/gen-manual-lists.py", line 216, in __init__ > self.root_config)) > File "/opt/buildroot/master/support/scripts/kconfiglib.py", line 214, in __init__ > self.top_block = self._parse_file(filename, None, None, None) > File "/opt/src/buildroot/master/support/scripts/kconfiglib.py", line 919, in _parse_file > return self._parse_block(line_feeder, None, parent, deps, visible_if_deps, res) > File "/opt/buildroot/master/support/scripts/kconfiglib.py", line 1114, in _parse_block > self.base_dir)) > IOError: /opt/buildroot/master/Config.in:490: sourced file "$BR2_EXTERNAL/Config.in" (expands to > "2_EXTERNAL/Config.in") not found. Perhaps base_dir > (argument to Config.__init__(), currently > "/opt/buildroot/master") is set to the wrong value. > docs/manual/manual.mk:2: recipe for target 'manual-update-lists' failed > make: *** [manual-update-lists] Error 1 > </snip> > > Reported-by: Ryan Barnett <rjbarnet@rockwellcollins.com> > Signed-off-by: Samuel Martin <s.martin49@gmail.com> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > support/scripts/kconfiglib.py | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Ryan Barnett <rjbarnet@rockwellcollins.com>
On 05/12/13 00:19, Yann E. MORIN wrote: > Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr> If you've reviewed it and tested it, you would commit it if you had commit access, right? So this could actually be an Acked-by, right? Or is my understanding of these tags incorrect? Regards, Arnout
Arnout, All, On 2013-12-05 19:12 +0100, Arnout Vandecappelle spake thusly: > On 05/12/13 00:19, Yann E. MORIN wrote: > >Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > >Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > > If you've reviewed it and tested it, you would commit it if you had commit > access, right? So this could actually be an Acked-by, right? Or is my > understanding of these tags incorrect? I'm following the definitions of Documentation/SubmittingPatches in my Linux kernel tree. For example, I refer to: Acked-by: does not necessarily indicate acknowledgement of the entire patch. Reviewed-by:, instead, indicates that the patch has been reviewed and found acceptable according to the Reviewer's Statement [--SNIP statement--] So, by providing both Reviewed-by and Tested by, I am explicitly stating that I did a review of the patch, and I tested it. Which, from my understanding, Acked-by does. Also, I do not believe to be in a position to provide my Acked-by on the core infrastructure, which is rather Thomas' domain. So, Thomas would be right to provide his Acked-by on such patches (but obviously he can't on those, since he's the author). Regards, Yann E. MORIN.
Arnout, All, On 2013-12-05 21:12 +0100, Yann E. MORIN spake thusly: > On 2013-12-05 19:12 +0100, Arnout Vandecappelle spake thusly: > > On 05/12/13 00:19, Yann E. MORIN wrote: > > >Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > > >Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr> > > > > If you've reviewed it and tested it, you would commit it if you had commit > > access, right? So this could actually be an Acked-by, right? Or is my > > understanding of these tags incorrect? > > I'm following the definitions of Documentation/SubmittingPatches in my > Linux kernel tree. > > For example, I refer to: > Acked-by: does not necessarily indicate acknowledgement of the entire > patch. > > Reviewed-by:, instead, indicates that the patch has been reviewed > and found acceptable according to the Reviewer's Statement > [--SNIP statement--] > > So, by providing both Reviewed-by and Tested by, I am explicitly stating > that I did a review of the patch, and I tested it. Which, from my > understanding, Acked-by does. ... Acked-by does *not*. Regards, Yann E. MORIN.
Here we go again... ;-) On Thu, Dec 5, 2013 at 9:13 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote: > Arnout, All, > > On 2013-12-05 21:12 +0100, Yann E. MORIN spake thusly: >> On 2013-12-05 19:12 +0100, Arnout Vandecappelle spake thusly: >> > On 05/12/13 00:19, Yann E. MORIN wrote: >> > >Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr> >> > >Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr> >> > >> > If you've reviewed it and tested it, you would commit it if you had commit >> > access, right? So this could actually be an Acked-by, right? Or is my >> > understanding of these tags incorrect? >> >> I'm following the definitions of Documentation/SubmittingPatches in my >> Linux kernel tree. >> >> For example, I refer to: >> Acked-by: does not necessarily indicate acknowledgement of the entire >> patch. >> >> Reviewed-by:, instead, indicates that the patch has been reviewed >> and found acceptable according to the Reviewer's Statement >> [--SNIP statement--] >> >> So, by providing both Reviewed-by and Tested by, I am explicitly stating >> that I did a review of the patch, and I tested it. Which, from my >> understanding, Acked-by does. > > ... Acked-by does *not*. We discussed this on the Buildroot developer day at FOSDEM 2012, see the report [1] and addition in the buildroot manual [2]. The manual says: Acked-by: Indicates that the patch can be committed. Tested-by: Indicates that the patch has been tested. It is useful but not necessary to add a comment about what has been tested. I must admit that in the mean time it has become common practice to also use Reviewed-by, so we'll need to clarify that. By no means authoritative, but here is what I mean when I add the following tags on a patch: - Tested-by: as in the manual: I performed some kind of test (typically described below the tag) on the patch. - Reviewed-by: I code-reviewed the patch and did my best in spotting problems, but I am not sufficiently familiar with the area touched to provide an Acked-by. This means that, although I reviewed the patch, there may be remaining problems that would be spotted by someone with more experience in that area. The detection of such problems should not mean that my Reviewed-by: was too hasty. - Acked-by: I code-reviewed the patch (note: not necessarily tested) and am familiar enough with the area touched that I can indicate it is a good patch. If someone else detects a serious problem with this patch afterwards, then this Acked-by may have been too hasty. So for me, Acked-by is stronger than Reviewed-by, but orthogonal to Tested-by. Note that my definition of Acked-by does not really rely on 'module owners', contrary to how Yann interprets it. For example in the case of the core infrastructure I don't believe that only ThomasP can provide an Acked-by. Several developers other than ThomasP have made good changes there, and are thus sufficiently knowledgeable to indicate their Ack. In my opinion, it is up to the maintainer to assess the weight of an Ack. He is free to wait until an ack by ThomasP, or not. (for the record: with the above I do not want to minimize ThomasP's contribution in this area, not at all. His work has been and is of great importance for the buildroot project, and I deeply respect it (and him) Best regards, Thomas [1] http://lists.busybox.net/pipermail/buildroot/2012-February/050371.html [2] http://buildroot.uclibc.org/downloads/manual/manual.html#_reviewing_testing_patches
On 06/12/13 09:03, Thomas De Schampheleire wrote: > Here we go again... ;-) > > On Thu, Dec 5, 2013 at 9:13 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote: >> Arnout, All, >> >> On 2013-12-05 21:12 +0100, Yann E. MORIN spake thusly: >>> On 2013-12-05 19:12 +0100, Arnout Vandecappelle spake thusly: >>>> On 05/12/13 00:19, Yann E. MORIN wrote: >>>>> Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr> >>>>> Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr> >>>> >>>> If you've reviewed it and tested it, you would commit it if you had commit >>>> access, right? So this could actually be an Acked-by, right? Or is my >>>> understanding of these tags incorrect? >>> >>> I'm following the definitions of Documentation/SubmittingPatches in my >>> Linux kernel tree. >>> >>> For example, I refer to: >>> Acked-by: does not necessarily indicate acknowledgement of the entire >>> patch. >>> >>> Reviewed-by:, instead, indicates that the patch has been reviewed >>> and found acceptable according to the Reviewer's Statement >>> [--SNIP statement--] >>> >>> So, by providing both Reviewed-by and Tested by, I am explicitly stating >>> that I did a review of the patch, and I tested it. Which, from my >>> understanding, Acked-by does. >> >> ... Acked-by does *not*. > > We discussed this on the Buildroot developer day at FOSDEM 2012, see > the report [1] and addition in the buildroot manual [2]. The manual > says: > > Acked-by: Indicates that the patch can be committed. > Tested-by: Indicates that the patch has been tested. It is useful but > not necessary to add a comment about what has been tested. > > I must admit that in the mean time it has become common practice to > also use Reviewed-by, so we'll need to clarify that. > > By no means authoritative, but here is what I mean when I add the > following tags on a patch: > - Tested-by: as in the manual: I performed some kind of test > (typically described below the tag) on the patch. > > - Reviewed-by: I code-reviewed the patch and did my best in spotting > problems, but I am not sufficiently familiar with the area touched to > provide an Acked-by. This means that, although I reviewed the patch, > there may be remaining problems that would be spotted by someone with > more experience in that area. The detection of such problems should > not mean that my Reviewed-by: was too hasty. > > - Acked-by: I code-reviewed the patch (note: not necessarily tested) > and am familiar enough with the area touched that I can indicate it is > a good patch. If someone else detects a serious problem with this > patch afterwards, then this Acked-by may have been too hasty. If this text makes it into the documentation, it gets my Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> :-) Now I do think I understand why Yann didn't give an Ack, but just a Reviewed: he doesn't feel familiar enough with the infrastructure to be really sure the patch is OK. You could also say: with Acked-by you are prepared to share the blame if something is wrong with the patch, with Reviewed-by you're not. Regards, Arnout > So for me, Acked-by is stronger than Reviewed-by, but orthogonal to Tested-by. > > Note that my definition of Acked-by does not really rely on 'module > owners', contrary to how Yann interprets it. For example in the case > of the core infrastructure I don't believe that only ThomasP can > provide an Acked-by. Several developers other than ThomasP have made > good changes there, and are thus sufficiently knowledgeable to > indicate their Ack. In my opinion, it is up to the maintainer to > assess the weight of an Ack. He is free to wait until an ack by > ThomasP, or not. > (for the record: with the above I do not want to minimize ThomasP's > contribution in this area, not at all. His work has been and is of > great importance for the buildroot project, and I deeply respect it > (and him) > > Best regards, > Thomas > > [1] http://lists.busybox.net/pipermail/buildroot/2012-February/050371.html > [2] http://buildroot.uclibc.org/downloads/manual/manual.html#_reviewing_testing_patches >
diff --git a/support/scripts/kconfiglib.py b/support/scripts/kconfiglib.py index 0704cc0..e40947c 100644 --- a/support/scripts/kconfiglib.py +++ b/support/scripts/kconfiglib.py @@ -2074,7 +2074,7 @@ set_re = re.compile(r"CONFIG_(\w+)=(.*)") unset_re = re.compile(r"# CONFIG_(\w+) is not set") # Regular expression for finding $-references to symbols in strings -sym_ref_re = re.compile(r"\$[A-Za-z_]+") +sym_ref_re = re.compile(r"\$[A-Za-z_][0-9A-Za-z_]*") # Integers representing symbol types UNKNOWN, BOOL, TRISTATE, STRING, HEX, INT = range(0, 6)