| Message ID | 20200825165341.520-1-luoyonggang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] meson: Fixes the ninjatool issue that E$$: are generated in Makefile.ninja | expand |
On 25/08/2020 17:53, luoyonggang@gmail.com wrote: > From: Yonggang Luo <luoyonggang@gmail.com> > > SIMPLE_PATH_RE should match the full path token. > Or the $ and : contained in path would not matched if the path are start with C:/ and E:/ > --- > scripts/ninjatool.py | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scripts/ninjatool.py b/scripts/ninjatool.py > index cc77d51aa8..6ca8be6f10 100755 > --- a/scripts/ninjatool.py > +++ b/scripts/ninjatool.py > @@ -55,7 +55,7 @@ else: > > PATH_RE = r"[^$\s:|]+|\$[$ :]|\$[a-zA-Z0-9_-]+|\$\{[a-zA-Z0-9_.-]+\}" > > -SIMPLE_PATH_RE = re.compile(r"[^$\s:|]+") > +SIMPLE_PATH_RE = re.compile(r"^[^$\s:|]+$") > IDENT_RE = re.compile(r"[a-zA-Z0-9_.-]+$") > STRING_RE = re.compile(r"(" + PATH_RE + r"|[\s:|])(?:\r?\n)?|.") > TOPLEVEL_RE = re.compile(r"([=:#]|\|\|?|^ +|(?:" + PATH_RE + r")+)\s*|.") I've tested this and it changes build.ninja so instead of Windows paths beginning C$$ they now begin C$ instead e.g.: build qemu-version.h: CUSTOM_COMMAND | C$:/msys64/home/Mark/qemu/scripts/qemu-version.sh PHONY I was expecting this not to work, however it seems in the next stage of transformation from build.ninja to Makefile.ninja the extra $ is removed correctly: qemu-version.h: qemu-version.h.stamp; @: qemu-version.h.stamp: C:/msys64/home/Mark/qemu/scripts/qemu-version.sh PHONY | ; ${ninja-command-restat} It feels like the extra $ shouldn't be present in build.ninja, but the patch does generate a Makefile.ninja that works. ATB, Mark.
On 8/25/20 11:53 AM, luoyonggang@gmail.com wrote: > From: Yonggang Luo <luoyonggang@gmail.com> > > SIMPLE_PATH_RE should match the full path token. > Or the $ and : contained in path would not matched if the path are start with C:/ and E:/ > --- Missing a Signed-off-by tag. Without that, we cannot apply it. Also missing a 0/4 cover letter; less essential, but useful for continuous integration tools and for replying to the series as a whole. More patch submission hints can be found at https://wiki.qemu.org/Contribute/SubmitAPatch
On Tue, Aug 25, 2020 at 11:26 PM Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > I've tested this and it changes build.ninja so instead of Windows paths beginning C$$ > they now begin C$ instead e.g.: > > build qemu-version.h: CUSTOM_COMMAND | > C$:/msys64/home/Mark/qemu/scripts/qemu-version.sh PHONY The patch should not change build.ninja in any way, but indeed it will fix the transformation so that the (correct) ninja quoting is removed. Paolo
On 26/08/2020 07:45, Paolo Bonzini wrote: > On Tue, Aug 25, 2020 at 11:26 PM Mark Cave-Ayland > <mark.cave-ayland@ilande.co.uk> wrote: >> I've tested this and it changes build.ninja so instead of Windows paths beginning C$$ >> they now begin C$ instead e.g.: >> >> build qemu-version.h: CUSTOM_COMMAND | >> C$:/msys64/home/Mark/qemu/scripts/qemu-version.sh PHONY > > The patch should not change build.ninja in any way, but indeed it will > fix the transformation so that the (correct) ninja quoting is removed. It definitely changes build.ninja here, with the escaping changing from C$$:\ to C$:\ in my tests. But if you're saying that $: with just a single $ is the correct escaping for colon then I guess the patch is fine (and indeed it works for me with the $ now being removed in the transformation to Makefile.ninja). ATB, Mark.
Il gio 27 ago 2020, 18:05 Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> ha scritto: > On 26/08/2020 07:45, Paolo Bonzini wrote: > > > On Tue, Aug 25, 2020 at 11:26 PM Mark Cave-Ayland > > <mark.cave-ayland@ilande.co.uk> wrote: > >> I've tested this and it changes build.ninja so instead of Windows paths > beginning C$$ > >> they now begin C$ instead e.g.: > >> > >> build qemu-version.h: CUSTOM_COMMAND | > >> C$:/msys64/home/Mark/qemu/scripts/qemu-version.sh PHONY > > > > The patch should not change build.ninja in any way, but indeed it will > > fix the transformation so that the (correct) ninja quoting is removed. > > It definitely changes build.ninja here, with the escaping changing from > C$$:\ to C$:\ > in my tests. Yes, the ^ makes no difference but I missed that it also adds a $. Thanks for testing!! Paolo if you're saying that $: with just a single $ is the correct > escaping for colon then I guess the patch is fine (and indeed it works for > me with > the $ now being removed in the transformation to Makefile.ninja). > > > ATB, > > Mark. > >
diff --git a/scripts/ninjatool.py b/scripts/ninjatool.py index cc77d51aa8..6ca8be6f10 100755 --- a/scripts/ninjatool.py +++ b/scripts/ninjatool.py @@ -55,7 +55,7 @@ else: PATH_RE = r"[^$\s:|]+|\$[$ :]|\$[a-zA-Z0-9_-]+|\$\{[a-zA-Z0-9_.-]+\}" -SIMPLE_PATH_RE = re.compile(r"[^$\s:|]+") +SIMPLE_PATH_RE = re.compile(r"^[^$\s:|]+$") IDENT_RE = re.compile(r"[a-zA-Z0-9_.-]+$") STRING_RE = re.compile(r"(" + PATH_RE + r"|[\s:|])(?:\r?\n)?|.") TOPLEVEL_RE = re.compile(r"([=:#]|\|\|?|^ +|(?:" + PATH_RE + r")+)\s*|.")
From: Yonggang Luo <luoyonggang@gmail.com> SIMPLE_PATH_RE should match the full path token. Or the $ and : contained in path would not matched if the path are start with C:/ and E:/ --- scripts/ninjatool.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)