All of lore.kernel.org
 help / color / mirror / Atom feed
From: Robin Jarry <robin.jarry@6wind.com>
To: buildroot@busybox.net
Subject: [Buildroot] [PATCH v2 3/4] pycompile: fix .pyc original source file paths
Date: Thu, 10 Sep 2020 09:29:10 +0200	[thread overview]
Message-ID: <20200910072910.lbjjuh2e4ncg5a7i@6wind.com> (raw)
In-Reply-To: <20200909203441.GR14354@scaer>

2020-09-09, Yann E. MORIN:
[snip]
> I am not python expert, but I tend to be using '{}'.format(..), and
> indeed, it seems that python considers the % formatting to be "old":
> 
>     https://docs.python.org/3/tutorial/inputoutput.html#old-string-formatting
> 
> Do you think you could switch to using str.format() ?

Sure, I have no strong opinion on the matter. I am more used to old
formatting since it is often shorter, but the new str.format() is less
ambiguous. I'll do that in a v3 today.

> So, back to this TARGET and PATH arguments. I find it pretty confusing
> the way they are described.
> 
> First, what TARGET used to be in the previous script is now something
> else, so keeping the naming is confusing. Also, 'target/' is a buildroot
> naming; I think we could get a better generic name: root
> 
> As for PATH, I find it strange that the metavar and variable do not
> match.
> 
>     parser.add_argument('root', metavar='ROOT',
>                         help='Path on the build machine that will be / at runtime')
>     parser.add_argument('dirs', metavar='DIR',
>                         help='Directory, relative to ROOT, to scan for py-files to compile')
> 
> So now, it should be possible to use as thus:
> 
>     python support/scripts/pycompile.py \
>         $(TARGET_DIR) \
>         usr/lib/python$(PYTHON_VERSION_MAJOR)
> 
> Or even (which I like better, because the DIR is absolute as seen at
> runtime too):
> 
>     python support/scripts/pycompile.py \
>         $(TARGET_DIR) \
>         /usr/lib/python$(PYTHON_VERSION_MAJOR)
> 
> But of course, dealing with path concatenation with an intermediate path
> that starts with a '/' is always tricky (I don't understand why upstream
> went that route...).
> 
>     for d in args.dirs:
>         abs = os.path.join(args.root, d[(1 if d[0] == '/' else 0):])
> 
> Thoughts?

I have another suggestion: changing the current TARGET argument into
a --strip-root "optional" argument (which will always be specified
anyway, but if not cross compiling, it can make sense not to use it).
It seems explicit enough to me and it avoids confusion with runtime/host
paths. What do you think?

I'll submit this in a v3 as well so we can discuss on something
concrete.

Thanks for the thorough review.

-- 
Robin

  reply	other threads:[~2020-09-10  7:29 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-04 11:29 [Buildroot] [PATCH] pycompile: fix .pyc original source file paths Julien Floret
2020-09-04 21:26 ` Yann E. MORIN
2020-09-04 21:32   ` Yann E. MORIN
2020-09-08  8:10 ` [Buildroot] [PATCH v2 0/4] pycompile: fix .pyc source paths + improvements Robin Jarry
2020-09-08  8:10   ` [Buildroot] [PATCH v2 1/4] pycompile: add main entry point Robin Jarry
2020-09-08  8:10   ` [Buildroot] [PATCH v2 2/4] pycompile: sort imports Robin Jarry
2020-09-08  8:10   ` [Buildroot] [PATCH v2 3/4] pycompile: fix .pyc original source file paths Robin Jarry
2020-09-09 20:34     ` Yann E. MORIN
2020-09-10  7:29       ` Robin Jarry [this message]
2020-09-08  8:10   ` [Buildroot] [PATCH v2 4/4] pycompile: add --verbose option Robin Jarry
2020-09-09 20:47     ` Yann E. MORIN
2020-09-09 19:53   ` [Buildroot] [PATCH v2 0/4] pycompile: fix .pyc source paths + improvements Yann E. MORIN
2020-09-10  7:45 ` [Buildroot] [PATCH v3 0/2] " Robin Jarry
2020-09-10  7:45   ` [Buildroot] [PATCH v3 1/2] support/scripts/pycompile: fix .pyc original source file paths Robin Jarry
2020-09-10  7:53     ` Robin Jarry
2020-09-10  7:45   ` [Buildroot] [PATCH v3 2/2] support/scripts/pycompile: add --verbose option Robin Jarry
2020-09-10  8:32 ` [Buildroot] [PATCH v4 0/2] pycompile: fix .pyc source paths + improvements Robin Jarry
2020-09-10  8:32   ` [Buildroot] [PATCH v4 1/2] support/scripts/pycompile: fix .pyc original source file paths Robin Jarry
2020-09-11 21:15     ` Yann E. MORIN
2020-09-12 11:44       ` Robin Jarry
2020-09-13  8:10         ` Yann E. MORIN
2020-09-13  9:03     ` Yann E. MORIN
2020-09-14  7:33       ` Robin Jarry
2020-09-15 18:46     ` Peter Korsgaard
2020-09-10  8:32   ` [Buildroot] [PATCH v4 2/2] support/scripts/pycompile: add --verbose option Robin Jarry
2020-09-13  9:03     ` Yann E. MORIN

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200910072910.lbjjuh2e4ncg5a7i@6wind.com \
    --to=robin.jarry@6wind.com \
    --cc=buildroot@busybox.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.