Add "lsource" for sourcing a list of files #75

dobairoland · 2019-09-18T12:20:39Z

Here is the PR I mentioned here: #71 (comment)

I hope it is worth of adding to your great library. I'm ready to make any necessary adjustments.

ulfalizer

Hello,

Maybe it could be included as a provisional feature for now, in case I think of some drawback or other way to do things.

It's a bit inconsistent in that there's no relative lsource, but at the same time I don't want to put in too much stuff that would never get used, so that might be fine.

Will look more tomorrow. Busy times lately with some other code.

kconfiglib.py

dobairoland · 2019-09-19T06:56:07Z

Thank you @ulfalizer for your suggestions and for the will to work toward this feature. I corrected the commit as per your suggestions.

It's a bit inconsistent in that there's no relative lsource, but at the same time I don't want to put in too much stuff that would never get used, so that might be fine.

Would you like more the idea that source, rsource and osource would accept a list as well (instead of adding lsource)?

ulfalizer · 2019-09-20T07:24:45Z

Thank you @ulfalizer for your suggestions and for the will to work toward this feature. I corrected the commit as per your suggestions.

Thanks!

Would you like more the idea that source, rsource and osource would accept a list as well (instead of adding lsource)?

One disadvantage is that it breaks paths with spaces in them. Probably very rare to have those, but you never know what people might depend on.

I'm fine with lsource for now. Even if I mark it provisional, I won't go changing stuff willy-nilly, because you probably shouldn't make people's lives too painful. In practice, it'll probably stay forever, unchanged. :)

Will look more over the weekend. Sorry for slowness. Should calm down soon.

dobairoland · 2019-09-20T08:44:04Z

One disadvantage is that it breaks paths with spaces in them. Probably very rare to have those, but you never know what people might depend on.

Yes, poor Windows users. :-) I forgot about those spaces. However, the list items could be separated with something else, e.g. semicolon.

Will look more over the weekend.

Thank you for your time!

dobairoland · 2019-09-20T13:15:03Z

I discovered an incompability with Python 2.

/~https://github.com/ulfalizer/Kconfiglib/blob/master/kconfiglib.py#L2472 token is unicode and not str, so it fails with kconfiglib.KconfigError: /home/dragon/esp/esp-idf/Kconfig:64: couldn't parse 'lsource "$COMPONENT_KCONFIGS_PROJBUILD"': expected string.

EDIT:
if type(token) not in [type(b''), type(u'')] seems to fix the issue.

ulfalizer · 2019-09-21T00:52:53Z

I discovered an incompability with Python 2.

/~https://github.com/ulfalizer/Kconfiglib/blob/master/kconfiglib.py#L2472 token is unicode and not str, so it fails with kconfiglib.KconfigError: /home/dragon/esp/esp-idf/Kconfig:64: couldn't parse 'lsource "$COMPONENT_KCONFIGS_PROJBUILD"': expected string.

Weird. I wonder how you ended up with unicode there. The library deliberately only deals with plain undecoded str on Python 2, because unicode-ifying everything would get messy and make 2/3 compatibility harder.

Could you check if os.environ["COMPONENT_KCONFIGS_PROJBUILD"] is a unicode string? That would explain it in that case, though I'm not sure how it'd happen, unless you're manually setting it.

The recommended syntax nowadays is this btw:

lsource "$(COMPONENT_KCONFIGS_PROJBUILD)"

That's using the new Kconfig preprocessor. The ()-less syntax is deprecated and only supported for backwards compatibility.

ulfalizer · 2019-09-21T12:05:34Z

I wonder if lsource should be in _OBL_SOURCE_TOKENS. Allowing a list of zero files seems more flexible, especially if they might be generated by a preprocessor function.

It should really be lsource, olsource, rlsource, and orlsource I guess, but it feels like overkill, and the code footprint for that is pretty large.

@dobairoland
Are all the components that can appear in /~https://github.com/espressif/esp-idf/blob/96b96ae244f360c84b71c8c3670e24a1186e9844/Kconfig#L67 already within the esp-idf repo by the way?

In that case, maybe you could do something like this instead:

if $(has-component,foo)
source "components/foo/Kconfig"
endif

if $(has-component,bar)
source "components/bar/Kconfig"
endif

...

has-component would be a Python preprocessor function (see

Kconfiglib/kconfiglib.py

Line 458 in f2ce282

Preprocessor user functions defined in Python

). It could be defined like this, in kconfigfunctions.py:

import os

import kconfiglib


def has_component(kconf, _, name):
    components = os.getenv("ESP_IDF_COMPONENTS")
    if components is None:
        raise kconfiglib.KconfigError(
            "{}:{}: $(has-component) requires the env. var. "
            "ESP_IDF_COMPONENTS to be set to a space-separated list of "
            "component names".format(kconf.filename, kconf.linenr))

    return "y" if name in components.split() else "n"


functions = {
    "has-component": (has_component, 1, 1)
}

Thoughts?

It's a bit more work on the esp-idf side, but on the upside, it makes it easy to grep for where files get sourced. The preprocessor functionality is pretty flexible.

dobairoland · 2019-09-23T10:39:51Z

Hi @ulfalizer. Thank you for taking the time and looking at this again.

Could you check if os.environ["COMPONENT_KCONFIGS_PROJBUILD"] is a unicode string?

Yes, it is unicode. Unicode characters in paths are not supported in Kconfiglib?

I wonder if lsource should be in _OBL_SOURCE_TOKENS. Allowing a list of zero files seems more flexible, especially if they might be generated by a preprocessor function.

I can remove it from there if you prefer that solution.

It should really be lsource, olsource, rlsource, and orlsource I guess, but it feels like overkill, and the code footprint for that is pretty large.

Yes, that seems to be too much.

Are all the components that can appear in /~https://github.com/espressif/esp-idf/blob/96b96ae244f360c84b71c8c3670e24a1186e9844/Kconfig#L67 already within the esp-idf repo by the way?

Unfortunately, no they are not. There are other frameworks which depend on ESP-IDF which has their components. Our customers have their own private components. I cannot even compile a complete list.

I've tried to add $(source-list,"$(COMPONENT_KCONFIGS)") istead of the proposed lsource and created kconfigfunctions.py:

def source_list(kconf, name, path_list):
    return '\n'.join(['source "{}"'.format(path) for path in path_list[1:-1].split()])

functions = {
    "source-list": (source_list, 1, 1),
}

The function returns:

source "/home/xy/esp/esp-idf/components/app_update/Kconfig.projbuild"
source "/home/xy/esp/esp-idf/components/bootloader/Kconfig.projbuild"
...

But unfortunately, this isn't working.

ulfalizer · 2019-09-23T11:16:35Z

Could you check if os.environ["COMPONENT_KCONFIGS_PROJBUILD"] is a unicode string?

Yes, it is unicode. Unicode characters in paths are not supported in Kconfiglib?

They are supported in the sense that Kconfiglib just avoids decoding any strings on Python 2, and I haven't seen unicode strings show up in os.environ before. Even if you do this, it stays as a regular str:

$ foo=ßö python
>>> import os
>>> os.environ["foo"]
'\xc3\x9f\xc3\xb6'

That's why I'm wondering how you ended up with unicode. Manually setting os.environ entries to unicode strings currently isn't supported at least, if that's what's going on.

Curious how it happened, so I can see if it's worth fixing. :)

I wonder if lsource should be in _OBL_SOURCE_TOKENS. Allowing a list of zero files seems more flexible, especially if they might be generated by a preprocessor function.

I can remove it from there if you prefer that solution.

Might be best if there's only gone be one variant.

Unfortunately, no they are not. There are other frameworks which depend on ESP-IDF which has their components. Our customers have their own private components. I cannot even compile a complete list.

Yeah, that makes it a bit messier.

Another option might be to generate a Kconfig file with a bunch of sources in it, though it isn't super pretty either. Something like Kconfig.modules.

Any chance that might work?

I've tried to add $(source-list,"$(COMPONENT_KCONFIGS)") istead of the proposed lsource and created kconfigfunctions.py:
def source_list(kconf, name, path_list):
    return '\n'.join(['source "{}"'.format(path) for path in path_list[1:-1].split()])

functions = {
    "source-list": (source_list, 1, 1),
}
The function returns:
source "/home/xy/esp/esp-idf/components/app_update/Kconfig.projbuild"
source "/home/xy/esp/esp-idf/components/bootloader/Kconfig.projbuild"
...
But unfortunately, this isn't working.

Nah, the preprocessor works at the single token level, so that won't work. It was designed like that when it was added to the C tools to prevent too much anarchy, though it'd be semi-handy here.

ulfalizer · 2019-09-23T11:44:44Z

Sorry for stalling, but lsource is starting to feel a bit like an oddball feature that wouldn't get used very often. When I put things in, I have to support them forever, and it has to mesh with whatever gets put in in the future.

I'm a bit afraid that someone will come along in the future and say "hey, I need to source a bunch of glob patterns from an environment variable, why isn't this globbing like everything else?", or "why isn't there a relative (or required) version of this?" At that point, it's hard to come up with a good reason why not, even though it starts to get into messy feature creep. :)

Maybe generating a Kconfig.modules wouldn't be too bad of a hack, if it's just for one spot. Could osource it so that it's still possible to run Kconfig without modules if you need to. Kconfig.modules would be fully general too, and won't suffer from feature creep like lsource might.

FWIW, Kconfig.modules is what Zephyr does.

I know you just want to get it in and get on with the work, so sorry about that. I have to make it work with everything that comes after it though, and deal with issues people have with it. :)

dobairoland · 2019-09-23T12:06:48Z

That's why I'm wondering how you ended up with unicode. Manually setting os.environ entries to unicode strings currently isn't supported at least, if that's what's going on.

It is getting de-serialized from a JSON object: /~https://github.com/espressif/esp-idf/blob/master/tools/kconfig_new/confgen.py#L224 and json.load returns unicode.

Another option might be to generate a Kconfig file with a bunch of sources in it, though it isn't super pretty either. Something like Kconfig.modules.

Thank you, I'll try this option.

Sorry for stalling, but lsource is starting to feel a bit like an oddball feature that wouldn't get used very often. When I put things in, I have to support them forever, and it has to mesh with whatever gets put in in the future.

I understand your point. Thank you for your suggestions and your work on this library!

ulfalizer · 2019-09-23T13:26:10Z

It is getting de-serialized from a JSON object: /~https://github.com/espressif/esp-idf/blob/master/tools/kconfig_new/confgen.py#L224 and json.load returns unicode.

I'll look into it. Thanks for the report!

Found theskumar/python-dotenv#176, though I can't find anything in the Python 2 os module docs that explicitly forbids it.

Do you still depend on Python 2?

dobairoland · 2019-09-23T13:52:59Z

Do you still depend on Python 2?

Yes, we have to support 2.7 in all of our active releases.

ulfalizer · 2019-09-23T13:58:04Z

Yes, we have to support 2.7 in all of our active releases.

Dug around a bit. I think it might be best to encode the strings on the esp-idf side before storing them into os.environ:

mypy --py2 foo.py prints
```
 foo.py:3: error: Incompatible types in assignment (expression has type "unicode", target has type "str")
```
for this script:
```
import os
os.environ["foo"] = u"bar"
```
mypy is an official Python thing, so might be a sign it's not intended. It supports allowing more than one type too.
Seems unicode values in os.environ can cause issues with subprocess on Windows (which Kconfiglib uses for $(shell), though that one's tricky to use portably anyway): Force environment variables to str with Python2 on Windows theskumar/python-dotenv#101

Maybe doing something similar to what this PR does in ESP-IDF would make sense.

dobairoland · 2019-10-04T08:58:56Z

I solved my issue by creating a temporary file with a list of source item.

Thank you for your help and kind suggestions.

ulfalizer requested changes Sep 19, 2019

View reviewed changes

kconfiglib.py Show resolved Hide resolved

kconfiglib.py Outdated Show resolved Hide resolved

kconfiglib.py Outdated Show resolved Hide resolved

kconfiglib.py Outdated Show resolved Hide resolved

kconfiglib.py Show resolved Hide resolved

Add "lsource" for sourcing a list of files

f893d5d

dobairoland force-pushed the master branch from 8659361 to f893d5d Compare September 19, 2019 06:45

dobairoland requested a review from ulfalizer September 19, 2019 06:51

dobairoland closed this Oct 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "lsource" for sourcing a list of files #75

Add "lsource" for sourcing a list of files #75

dobairoland commented Sep 18, 2019

ulfalizer left a comment

dobairoland commented Sep 19, 2019

ulfalizer commented Sep 20, 2019

dobairoland commented Sep 20, 2019

dobairoland commented Sep 20, 2019 •

edited

Loading

ulfalizer commented Sep 21, 2019 •

edited

Loading

ulfalizer commented Sep 21, 2019 •

edited

Loading

dobairoland commented Sep 23, 2019

ulfalizer commented Sep 23, 2019 •

edited

Loading

ulfalizer commented Sep 23, 2019 •

edited

Loading

dobairoland commented Sep 23, 2019

ulfalizer commented Sep 23, 2019

dobairoland commented Sep 23, 2019

ulfalizer commented Sep 23, 2019 •

edited

Loading

dobairoland commented Oct 4, 2019

Add "lsource" for sourcing a list of files #75

Add "lsource" for sourcing a list of files #75

Conversation

dobairoland commented Sep 18, 2019

ulfalizer left a comment

Choose a reason for hiding this comment

dobairoland commented Sep 19, 2019

ulfalizer commented Sep 20, 2019

dobairoland commented Sep 20, 2019

dobairoland commented Sep 20, 2019 • edited Loading

ulfalizer commented Sep 21, 2019 • edited Loading

ulfalizer commented Sep 21, 2019 • edited Loading

dobairoland commented Sep 23, 2019

ulfalizer commented Sep 23, 2019 • edited Loading

ulfalizer commented Sep 23, 2019 • edited Loading

dobairoland commented Sep 23, 2019

ulfalizer commented Sep 23, 2019

dobairoland commented Sep 23, 2019

ulfalizer commented Sep 23, 2019 • edited Loading

dobairoland commented Oct 4, 2019

dobairoland commented Sep 20, 2019 •

edited

Loading

ulfalizer commented Sep 21, 2019 •

edited

Loading

ulfalizer commented Sep 21, 2019 •

edited

Loading

ulfalizer commented Sep 23, 2019 •

edited

Loading

ulfalizer commented Sep 23, 2019 •

edited

Loading

ulfalizer commented Sep 23, 2019 •

edited

Loading