| Message ID | 1476383370-30650-1-git-send-email-sw@weilnetz.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 13 October 2016 at 19:29, Stefan Weil <sw@weilnetz.de> wrote: > scripts/tracetool generates a C preprocessor macro from the name of the > build directory. Any characters which are possible in a directory name > but not allowed in a macro name must be substituted, otherwise builds > will fail. > > Signed-off-by: Stefan Weil <sw@weilnetz.de> > --- > > I had problems with a build directory of the form "host,variant". > Is this fix needed for stable, too? Why does it need to care about the build directory name at all? Ideally builds should be entirely deterministically reproducible whatever the path to the source or build directory names is... thanks -- PMM
On Thu, Oct 13, 2016 at 07:36:07PM +0100, Peter Maydell wrote: > On 13 October 2016 at 19:29, Stefan Weil <sw@weilnetz.de> wrote: > > scripts/tracetool generates a C preprocessor macro from the name of the > > build directory. Any characters which are possible in a directory name > > but not allowed in a macro name must be substituted, otherwise builds > > will fail. > > > > Signed-off-by: Stefan Weil <sw@weilnetz.de> > > --- > > > > I had problems with a build directory of the form "host,variant". > > Is this fix needed for stable, too? > > Why does it need to care about the build directory name at all? > Ideally builds should be entirely deterministically reproducible > whatever the path to the source or build directory names is... It's trying to construct the relative path from the source directory to a trace-events file (inside the source directory). This is used so that block/trace-event macros have BLOCK in their name while net/trace-events have NET, etc. It does not care about the build directory or source directory path per se. Therefore deterministic builds shouldn't be a problem if the code is working correctly ;). Stefan
On Thu, Oct 13, 2016 at 08:29:30PM +0200, Stefan Weil wrote: > scripts/tracetool generates a C preprocessor macro from the name of the > build directory. Any characters which are possible in a directory name > but not allowed in a macro name must be substituted, otherwise builds > will fail. > > Signed-off-by: Stefan Weil <sw@weilnetz.de> > --- > > I had problems with a build directory of the form "host,variant". > Is this fix needed for stable, too? > > Regards > Stefan W. > > scripts/tracetool.py | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scripts/tracetool.py b/scripts/tracetool.py > index 629b259..fe9c9e9 100755 > --- a/scripts/tracetool.py > +++ b/scripts/tracetool.py > @@ -70,7 +70,7 @@ def make_group_name(filename): > > if dirname == "": > return "common" > - return re.sub(r"/|-", "_", dirname) > + return re.sub(r"[^A-Za-z0-9]", "_", dirname) dirname should only contain QEMU source tree subdirectory paths (e.g. hw/net). How did you get a comma in there? I tried an out-of-tree build in a directory called 'a,b' but it doesn't trigger the issue. I'd like to understand how to reproduce the problem in case I missed something. Stefan
On 14/10/2016 11:53, Stefan Hajnoczi wrote: > > Why does it need to care about the build directory name at all? > > Ideally builds should be entirely deterministically reproducible > > whatever the path to the source or build directory names is... > > It's trying to construct the relative path from the source directory to > a trace-events file (inside the source directory). This is used so that > block/trace-event macros have BLOCK in their name while net/trace-events > have NET, etc. But Stefan (Weil) is right, it's actually including the path to the build directory if you're building with objdir!=srcdir. It should not need that. Paolo
On 10/14/16 12:01, Stefan Hajnoczi wrote: > dirname should only contain QEMU source tree subdirectory paths (e.g. > hw/net). How did you get a comma in there? > > I tried an out-of-tree build in a directory called 'a,b' but it doesn't > trigger the issue. > > I'd like to understand how to reproduce the problem in case I missed > something. > > Stefan I can reproduce it like this from the QEMU base directory: $ mkdir a,b $ cd a,b $ ../configure [...] $ make [...] make: *** [trace/generated-tracers.o] Error 1 $ head trace/generated-tracers.h /* This file is autogenerated by tracetool, do not edit. */ #ifndef TRACE_A,B_GENERATED_TRACERS_H #define TRACE_A,B_GENERATED_TRACERS_H Stefan
On Fri, Oct 14, 2016 at 10:05:18PM +0200, Stefan Weil wrote: > On 10/14/16 12:01, Stefan Hajnoczi wrote: > > dirname should only contain QEMU source tree subdirectory paths (e.g. > > hw/net). How did you get a comma in there? > > > > I tried an out-of-tree build in a directory called 'a,b' but it doesn't > > trigger the issue. > > > > I'd like to understand how to reproduce the problem in case I missed > > something. > > > > Stefan > > I can reproduce it like this from the QEMU base directory: > > $ mkdir a,b > $ cd a,b > $ ../configure > [...] > $ make > [...] > make: *** [trace/generated-tracers.o] Error 1 > $ head trace/generated-tracers.h > /* This file is autogenerated by tracetool, do not edit. */ > > #ifndef TRACE_A,B_GENERATED_TRACERS_H > #define TRACE_A,B_GENERATED_TRACERS_H Interesting! I missed it because my a,b directory was not inside the source directory. Stefan
13.10.2016 21:29, Stefan Weil wrote: > scripts/tracetool generates a C preprocessor macro from the name of the > build directory. Any characters which are possible in a directory name > but not allowed in a macro name must be substituted, otherwise builds > will fail. Applied to -trivial, thank you! /mjt
On 16 October 2016 at 15:31, Michael Tokarev <mjt@tls.msk.ru> wrote: > 13.10.2016 21:29, Stefan Weil wrote: >> scripts/tracetool generates a C preprocessor macro from the name of the >> build directory. Any characters which are possible in a directory name >> but not allowed in a macro name must be substituted, otherwise builds >> will fail. > > Applied to -trivial, thank you! Consensus on this and the other thread seemed to be that we should just fix the bug where tracetool is putting the build directory name into symbols rather than working around that... thanks -- PMM
Am 16.10.2016 um 21:04 schrieb Peter Maydell: > On 16 October 2016 at 15:31, Michael Tokarev <mjt@tls.msk.ru> wrote: >> 13.10.2016 21:29, Stefan Weil wrote: >>> scripts/tracetool generates a C preprocessor macro from the name of the >>> build directory. Any characters which are possible in a directory name >>> but not allowed in a macro name must be substituted, otherwise builds >>> will fail. >> >> Applied to -trivial, thank you! > > Consensus on this and the other thread seemed to be > that we should just fix the bug where tracetool is > putting the build directory name into symbols > rather than working around that... > > thanks > -- PMM > IMHO the patch can be applied nevertheless, as it uses a clearer regular expression for characters allowed in macro names (not one that is specific for the current QEMU code). Stefan
17.10.2016 07:43, Stefan Weil wrote: > Am 16.10.2016 um 21:04 schrieb Peter Maydell: >> On 16 October 2016 at 15:31, Michael Tokarev <mjt@tls.msk.ru> wrote: >>> 13.10.2016 21:29, Stefan Weil wrote: >>>> scripts/tracetool generates a C preprocessor macro from the name of the >>>> build directory. Any characters which are possible in a directory name >>>> but not allowed in a macro name must be substituted, otherwise builds >>>> will fail. >>> >>> Applied to -trivial, thank you! >> >> Consensus on this and the other thread seemed to be >> that we should just fix the bug where tracetool is >> putting the build directory name into symbols >> rather than working around that... >> >> thanks >> -- PMM > > IMHO the patch can be applied nevertheless, as it uses a clearer > regular expression for characters allowed in macro names > (not one that is specific for the current QEMU code). So, what's the thing here? Once again I hit this bug today, when my usual build environment doesn't work: ./trace/generated-tracers.h:3:15: error: extra tokens at end of #ifndef directive [-Werror] #ifndef TRACE_.B64_GENERATED_TRACERS_H ^ and this patch fixes the issue for me. Thanks, /mjt
On 28 October 2016 at 15:57, Michael Tokarev <mjt@tls.msk.ru> wrote: > 17.10.2016 07:43, Stefan Weil wrote: >> Am 16.10.2016 um 21:04 schrieb Peter Maydell: >>> On 16 October 2016 at 15:31, Michael Tokarev <mjt@tls.msk.ru> wrote: >>>> 13.10.2016 21:29, Stefan Weil wrote: >>>>> scripts/tracetool generates a C preprocessor macro from the name of the >>>>> build directory. Any characters which are possible in a directory name >>>>> but not allowed in a macro name must be substituted, otherwise builds >>>>> will fail. >>>> >>>> Applied to -trivial, thank you! >>> >>> Consensus on this and the other thread seemed to be >>> that we should just fix the bug where tracetool is >>> putting the build directory name into symbols >>> rather than working around that... >>> >>> thanks >>> -- PMM >> >> IMHO the patch can be applied nevertheless, as it uses a clearer >> regular expression for characters allowed in macro names >> (not one that is specific for the current QEMU code). > > So, what's the thing here? I think we concluded that the underlying problem was a bit tricky to fix and we should apply this workaround in the meantime. thanks -- PMM
diff --git a/scripts/tracetool.py b/scripts/tracetool.py index 629b259..fe9c9e9 100755 --- a/scripts/tracetool.py +++ b/scripts/tracetool.py @@ -70,7 +70,7 @@ def make_group_name(filename): if dirname == "": return "common" - return re.sub(r"/|-", "_", dirname) + return re.sub(r"[^A-Za-z0-9]", "_", dirname) def main(args): global _SCRIPT
scripts/tracetool generates a C preprocessor macro from the name of the build directory. Any characters which are possible in a directory name but not allowed in a macro name must be substituted, otherwise builds will fail. Signed-off-by: Stefan Weil <sw@weilnetz.de> --- I had problems with a build directory of the form "host,variant". Is this fix needed for stable, too? Regards Stefan W. scripts/tracetool.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)