From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 620C57030C; Tue, 9 Feb 2021 17:46:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 620C57030C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1612881975; bh=QPo8utydfs3m7uQJo2E0Kh/6E1qHJwwP6jDrOL6QMaU=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=zMkOIlMphUARl4NddkA7L0sQxQ7ST5+zjP3VF9yt8PWHRBFsgvXRDsSux543kGiNA tv7FgMXomD38HMEQS4eFFfCfKuudUPCsHcfdyDszMQ1EUsO6MqKJf8dAuEQZuluOFM TiI1/BBtNhnbn8m2Wf2xx1car5xtPRicpBOtWYOA= Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 3624D7030C for ; Tue, 9 Feb 2021 17:46:14 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3624D7030C Received: by smtp44.i.mail.ru with esmtpa (envelope-from ) id 1l9UHK-0007L9-1C; Tue, 09 Feb 2021 17:46:10 +0300 Date: Tue, 9 Feb 2021 17:45:28 +0300 To: Igor Munkin Message-ID: <20210209144528.GA9361@root> References: <20210209113852.GA23319@root> <20210209124731.GH5448@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210209124731.GH5448@tarantool.org> X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD953AC099BC0052A9C55152E5B36F7009490E3DBC75EBFD4CE182A05F5380850405D3FF926F1467BFB788CB552D102369FDBBA7C7E815CB0C1AC9455C54C5B57DB X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7560ADFC8AADDC107EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637B8A896DD3ADA3FA48638F802B75D45FF5571747095F342E8C7A0BC55FA0FE5FC649E0843118E761994FF314FD1446CD7D7EE39020E3F473C389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C091DAD9F922AA71188941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B6F459A8243F1D1D44CC7F00164DA146DA6F5DAA56C3B73B23C77107234E2CFBA567F23339F89546C55F5C1EE8F4F765FCEB1A37DF9DABAA8F75ECD9A6C639B01BBD4B6F7A4D31EC0BC0CAF46E325F83A522CA9DD8327EE4930A3850AC1BE2E73528A6D463EDFD0DBBC4224003CC836476C0CAF46E325F83A50BF2EBBBDD9D6B0F5D41B9178041F3E72623479134186CDE6BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A5ED3EEBD830D25B67D6665AF0700387A730FFF325F49BEF5ED59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34F1ADD4D8CD3C81CE805074988A912B79BE8D422711B15692C8B9FB9F76941E2C26D48E8C23EAE1A31D7E09C32AA3244CAF54AF9B83B190714D30CD08D3E5A9C195A9E0DC41E9A4CFFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojg67HQLCbniK1iM5ALxnUlg== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB43D3B89B3949A6494657F1441D9AD98F5C7BFCE82F7DD5E2CF2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 1/5] build: preserve the original build system X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Igor, On 09.02.21, Igor Munkin wrote: > Sergey, > > Thanks for your review! > > On 09.02.21, Sergey Kaplun wrote: > > Hi, Igor! > > > > Thanks for the patch! > > LGTM, except 4 comments about memory profiler parser below. > > I have several questions, so postponed your Ack a bit. No problem. > > > > > On 02.02.21, Igor Munkin wrote: > > > Since the build machinery is going to be ported to CMake there would be > > > Makefile names clashing. This change renames the original build system > > > (and a couple of auxiliary files requiring configuring) to keep all this > > > machinery working. > > > > > > As a result of these changes one need to explicitly specify the Makefile > > > in the build command: > > > | make -f Makefile.original > > > > > > Needed for tarantool/tarantool#4862 > > > > > > Signed-off-by: Igor Munkin > > > --- > > > Makefile => Makefile.original | 44 +++++++++++++-------- > > > etc/{luajit.pc => luajit.pc.in} | 4 +- > > > src/{Makefile.dep => Makefile.dep.original} | 0 > > > src/{Makefile => Makefile.original} | 4 +- > > > tools/luajit-parse-memprof | 9 ----- > > > tools/luajit-parse-memprof.in | 6 +++ > > > 6 files changed, 38 insertions(+), 29 deletions(-) > > > rename Makefile => Makefile.original (85%) > > > rename etc/{luajit.pc => luajit.pc.in} (91%) > > > rename src/{Makefile.dep => Makefile.dep.original} (100%) > > > rename src/{Makefile => Makefile.original} (99%) > > > delete mode 100755 tools/luajit-parse-memprof > > > create mode 100644 tools/luajit-parse-memprof.in > > > > > > diff --git a/Makefile b/Makefile.original > > > similarity index 85% > > > rename from Makefile > > > rename to Makefile.original > > > index 61967df..b85d4bf 100644 > > > --- a/Makefile > > > +++ b/Makefile.original > > > @@ -85,10 +85,10 @@ INSTALL_X= install -m 0755 > > > > > > > > > @@ -185,13 +185,25 @@ uninstall: > > > > > > ############################################################################## > > > > > > -amalg: > > > +amalg: tools > > > @echo "Building LuaJIT $(VERSION)" > > > - $(MAKE) -C src amalg > > > + $(MAKE) -C src -f Makefile.original amalg > > > > > > clean: > > > - $(MAKE) -C src clean > > > + $(RM) tools/$(FILE_TMEMPROF) > > > + $(MAKE) -C src -f Makefile.original clean > > > > > > -.PHONY: all install amalg clean > > > +tools: tools/$(FILE_TMEMPROF) > > > + > > > +# FIXME: This is an ugly hack to manually configure an auxiliary > > > +# tools/luajit-parse-memprof. I hope this file will have gone away > > > > 1. I suggest the following rewording ("I hope" is too personal :)): > > s/I hope this file will have gone away/This file should go away/ > > Fixed, squashed, force-pushed to the branch. Diff is below: Thanks! > > ================================================================================ > > diff --git a/Makefile.original b/Makefile.original > index b85d4bf..c3172f0 100644 > --- a/Makefile.original > +++ b/Makefile.original > @@ -196,8 +196,8 @@ clean: > tools: tools/$(FILE_TMEMPROF) > > # FIXME: This is an ugly hack to manually configure an auxiliary > -# tools/luajit-parse-memprof. I hope this file will have gone away > -# in scope of https://github.com/tarantool/tarantool/issues/5688. > +# tools/luajit-parse-memprof. This file should go away in scope of > +# https://github.com/tarantool/tarantool/issues/5688. > tools/$(FILE_TMEMPROF): > @sed -e "s|@LUAJIT_TOOLS_DIR@|$(realpath tools)|" \ > -e "s|@LUAJIT_TOOLS_BIN@|$(realpath src/luajit)|" \ > > ================================================================================ > > > > > > +# in scope of https://github.com/tarantool/tarantool/issues/5688. > > > +tools/$(FILE_TMEMPROF): > > > + @sed -e "s|@LUAJIT_TOOLS_DIR@|$(realpath tools)|" \ > > > + -e "s|@LUAJIT_TOOLS_BIN@|$(realpath src/luajit)|" \ > > > > 2. @LUAJIT_BIN@ looks more convenient, doesn't it? > > It doesn't for me. Why does the current naming look inconvenient to you? What does LUAJIT_TOOLS_BIN mean? I confused by TOOLS part. > > > > > > + $@.in > $@ > > > + @chmod +x $@ > > > + > > > +.PHONY: all install amalg clean tools > > > > > > ############################################################################## > > > > > > diff --git a/tools/luajit-parse-memprof.in b/tools/luajit-parse-memprof.in > > > new file mode 100644 > > > index 0000000..8867202 > > > --- /dev/null > > > +++ b/tools/luajit-parse-memprof.in > > > @@ -0,0 +1,6 @@ > > > +#!/bin/bash > > > +# > > > +# Launcher for memprof parser. > > > + > > > +LUA_PATH="@LUAJIT_TOOLS_DIR@/?.lua;;" \ > > > + @LUAJIT_TOOLS_BIN@ @LUAJIT_TOOLS_DIR@/memprof.lua $@ > > > > 3. Unfortunately, your solution doesn't work for me: > > Crap, I believe I've fixed this. > > > > > | $ git log --oneline -n1 > > | 7badb7e (HEAD) build: preserve the original build system > > | $ make -f Makefile.original -j > > | ==== Building LuaJIT 2.1.0-beta3 ==== > > | ... > > | ==== Successfully built LuaJIT 2.1.0-beta3 ==== > > > > Side note: I suppose it is nothing bad to add corresponding lines about > > tools building. At least it simplifies build debugging. > > Feel free to ignore. > > One can adjust this for debugging. Otherwise there is no need to see > this output. We can use those hacks with Q/E, but I hope this part will > have gone in the nearest future. OK, never mind. > > > > > | $ tools/luajit-parse-memprof /tmp/memprof_memleak.bin; echo -e "\n"; cat tools/luajit-parse-memprof > > | tools/luajit-parse-memprof: line 5: /home/burii/reviews/luajit/cmake/tools/memprof.lua: Permission denied > > | > > | #!/bin/bash > > | # > > | # Launcher for memprof parser. > > | > > | LUA_PATH="/home/burii/reviews/luajit/cmake/tools/?.lua;;" \ > > | /home/burii/reviews/luajit/cmake/tools/memprof.lua $@ > > > > This patch is working for me. But I am not bash|make guru :). > > Heh, try your patch on a clean repo: unfortunately it works neither. The > root cause is the lack of on configuration phase. I added Hmm, it's weird. `realpath` manual declares: | all but the last component must exist To be honest, I don't understand why it fails even in the first version -- obviously, exists. Anyway, LGTM. > the corresponding dependency for this rule. Fixed, squashed, > force-pushed to the branch. Diff is below: > > ================================================================================ > > diff --git a/Makefile.original b/Makefile.original > index c3172f0..33dc2ed 100644 > --- a/Makefile.original > +++ b/Makefile.original > @@ -198,7 +198,7 @@ tools: tools/$(FILE_TMEMPROF) > # FIXME: This is an ugly hack to manually configure an auxiliary > # tools/luajit-parse-memprof. This file should go away in scope of > # https://github.com/tarantool/tarantool/issues/5688. > -tools/$(FILE_TMEMPROF): > +tools/$(FILE_TMEMPROF): src/luajit > @sed -e "s|@LUAJIT_TOOLS_DIR@|$(realpath tools)|" \ > -e "s|@LUAJIT_TOOLS_BIN@|$(realpath src/luajit)|" \ > $@.in > $@ > > ================================================================================ > > After this patch everything is fine. > > > > > | diff --git a/Makefile.original b/Makefile.original > > | index b85d4bf..877398a 100644 > > | --- a/Makefile.original > > | +++ b/Makefile.original > > | @@ -199,8 +199,8 @@ tools: tools/$(FILE_TMEMPROF) > > | # tools/luajit-parse-memprof. I hope this file will have gone away > > | # in scope of https://github.com/tarantool/tarantool/issues/5688. > > | tools/$(FILE_TMEMPROF): > > | - @sed -e "s|@LUAJIT_TOOLS_DIR@|$(realpath tools)|" \ > > | - -e "s|@LUAJIT_TOOLS_BIN@|$(realpath src/luajit)|" \ > > | + @sed -e "s|@LUAJIT_TOOLS_DIR@|`realpath tools`|" \ > > | + -e "s|@LUAJIT_TOOLS_BIN@|`realpath src/luajit`|" \ > > | $@.in > $@ > > | @chmod +x $@ > > | > > > > 4. Please, create separate commit to tools-related changes. > > For what? I don't understand existing of these changes (but I like them :)) from commit message. Is it mentioned in the following part? | (and a couple of auxiliary files requiring configuring) Is it better to mention directly tools files here instead "auxiliary"? > > > Also, I suppose, it would be nice to add > > to <.gitignore> as far as it's generated. These lines may be confusing: > > This is added in the next patch, but I'm not against moving it to this > one. Fixed, squashed, force-pushed to the branch. Diff is below: Thanks! But one nitpick, feel free to ignore. > > ================================================================================ > > diff --git a/.gitignore b/.gitignore > index 1a07bf7..fc9c31e 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -9,3 +9,6 @@ > *.dmp > *.swp > .tags > + > +# Configured runner for LuaJIT memprof parser Nit: s/parser/parser./ Feel free to ignore. > +tools/luajit-parse-memprof > > ================================================================================ > > > > > | $ git status > > | HEAD detached at 7badb7e > > | Untracked files: > > | (use "git add ..." to include in what will be committed) > > | tools/luajit-parse-memprof > > > > > -- > > > 2.25.0 > > > > > > > -- > > Best regards, > > Sergey Kaplun > > -- > Best regards, > IM -- Best regards, Sergey Kaplun