-
Notifications
You must be signed in to change notification settings - Fork 7.5k
Add a Python menuconfig implementation #7174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a Python menuconfig implementation #7174
Conversation
CC @carlescufi |
2ca0c89
to
c5aa8bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Running on Windows requires first installing an appropriate wheel from
https://www.lfd.uci.edu/~gohlke/pythonlibs/#curses. Running python -VV
gives the Python version and bitness, to know which wheel to install.
That will be documented later."
The doc's should be updated in this PR.
See #5847 for some development screenshots. |
@@ -22,6 +22,7 @@ set(kconfig_target_list | |||
config | |||
gconfig | |||
menuconfig | |||
pymenuconfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about 'menu' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My plan was to rename it to 'menuconfig' later, once we get rid of the C tools. That's what people are used to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this would be incosistent with the other Kconfig-frontends I guess ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with renaming to menuconfig
later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not overcomplicate things. Let's replace mconf
with pymenuconfig
in this PR, and keep the "menuconfig" name
I thought I could update them once we get rid of the C tools and all the naming and stuff is set. I could put together some docs that describe this "staging" version as well though if you want. |
CC @carlescufi |
Nope. We need doc's now to know how bad it will be for Windows users to install and for Windows users to be able to test this. Also, it enforces that we don't have features with undocumented dependencies in master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires an update to kconfig-usage.cmake.
Alternatively, we could try to deprecate kconfig-usage.cmake entirely. It was ported from Kbuild, but I'm not sure if it is found to be useful.
Codecov Report
@@ Coverage Diff @@
## master #7174 +/- ##
==========================================
+ Coverage 55.02% 58.54% +3.51%
==========================================
Files 477 464 -13
Lines 51749 47400 -4349
Branches 9951 8783 -1168
==========================================
- Hits 28477 27748 -729
+ Misses 19306 15821 -3485
+ Partials 3966 3831 -135
Continue to review full report at Codecov.
|
@ulfalizer I agree that the docs should be part of this PR. In fact maybe we should go ahead with replacing |
@SebastianBoe @carlescufi I added instructions for installing the curses wheel on Windows. More documentation is needed as well, but it's a start. |
|
||
.. code-block:: console | ||
|
||
C:\...\zephyr> python -VV |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the prompt c:\...\zephyr>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, that makes it less clear that what follows it is the output, but alright.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but makes it harder to copy-paste and we have this convention all over the documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... yeah, think I would want to split it up in that case.
I think I prefer for it to be immediately obvious rather than perfectly consistent. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then do it as follows:
Type the following command:
python -VV
and check for the output, which should be something like:;
Python 3.6.5 (v3.6.5:f59c0932b4, Mar 28 2018, 16:07:46) [MSC v.1900 32 bit (Intel)]
|
||
The output in this case indicates Python 3.6, 32-bit. | ||
|
||
Next, go to `this repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace "repository" with "web page"
@@ -22,6 +22,7 @@ set(kconfig_target_list | |||
config | |||
gconfig | |||
menuconfig | |||
pymenuconfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not overcomplicate things. Let's replace mconf
with pymenuconfig
in this PR, and keep the "menuconfig" name
bfd4789
to
7c5ef63
Compare
@carlescufi Thinking of removing the C tools completely later, once |
@SebastianBoe Do you know which of the other ones people use in practice? |
"What about trimming kconfig-usage.cmake down to the essentials, e.g. menuconfig and oldconfig? defconfig files can be generated from within the menuconfig interface now." Sounds good. "Do you know which of the other ones people use in practice?" I have never heard of anyone ever using anything other than xconfig and menuconfig. But it is of course impossible to know. |
scripts/kconfig/menuconfig.py
Outdated
COLOR_BRIGHT_RED = 12 | ||
COLOR_BRIGHT_MAGENTA = 13 | ||
COLOR_BRIGHT_YELLOW = 14 | ||
COLOR_BRIGHT_WHITE = 15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do these values come from? A comment would be nice, that also gives an overview of the issue.
This is probably an RFC, but I got curious anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on the Windows Command prompt:
color /?
scripts/kconfig/menuconfig.py
Outdated
@@ -124,6 +124,25 @@ def _init_styles(): | |||
global _INFO_BOT_SEP_STYLE | |||
global _INFO_HELP_STYLE | |||
|
|||
if sys.platform.startswith('win'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like if platform.system() == "Windows"
might be better.
Even then it's probably mostly up to the terminal, so gets a bit iffy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is taken from another toolkit, feel free to change
@SebastianBoe Do you know of any people that might be "heavy" |
@dbkinder the documentation has been removed from this PR |
@SebastianBoe mind taking another look? See my comment about the documentation, which we don't want to write twice. Since we want to include the wheels in PyPi later, we want to have this "undocumented" first in order to move forward with this. |
I didn't have chance to follow this thru, and it seems there're enough people on this, so I'm sure it was already said and/or considered, so please treat the below just as "+1's" in this case:
Again, sorry if all of the above is obvious and was already considered. Just my 2 cents. Thanks. |
Documentation changes were removed, so nothing to review :(
@dbkinder |
Might be to decrease the maintenance burden, since there are so many wheels on that page. @cgohlke is the maintainer by the way.
Sounds like a good idea. Will need to check with @cgohlke whether he's okay with mirrors and custom-built versions of the wheels being hosted elsewhere too. |
AFAIK, the Python dependency installation procedure on all platforms right now (for mandatory dependencies) is roughly:
It would be nice to be able to avoid having to modify that with "... and if you're on windows here's another set of requirements", i.e. try to keep just one requirements.txt. If that proves impossible, perhaps a scripts/win-requirements.txt is in order to initially contain this curses-windows dependency? |
Yeah, would be nicer if it was automatic, if we can get that reliable somehow. Manual is better than broken automatic at least. Switching over to some other toolkit would be way less work than doing the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon reflection, I really don't feel comfortable merging this without a clear and convenient installation solution being available for Windows users. Preferably one that adds no extra steps.
As discussed at the TSC meeting, can we have (zephyrproject-rtos owned on GitHub) infrastructure ready for installing the Windows wheel via pip before merging?
Right now, the curses solution is more difficult for Windows users to install than solutions based on other libraries (blessings, python-prompt-toolkit, etc.). I know most of us aren't on Windows (and I'm not either), but this is a basic workflow item, and we've prioritized making Windows a first class citizen, so it just doesn't feel right to merge as-is.
@mbolivar Then, once it's ready, including the installation parts, we make the final switch. |
I understand, and I made the same argument in one of my PRs (#6832) regarding flashing and debugging. But I have become convinced that Zephyr has hit the point where we should slow down on eagerly merging things before they are ready. I've marked my own PR with the "do not merge" tag as well in an effort to be consistent about this. I have felt the pain of Zephyr's historical pattern of API churn over the past 16 months of working with it, and while I previously accepted it as a consequence of immaturity, I'm glad to see increased willingness among the project maintainers to put the brakes on that, even if it inconveniences me personally. |
@mbolivar Having the experimental |
It's not about that. It's about how to decide whether or not to merge something. From my perspective, merging code which implements critical workflow functionality for all Zephyr users and developers should be done when that code solves the problem on all supported platforms in a way we're all happy with. I think the Windows issue is a blocker here. |
@mbolivar It's a nicer workflow than adding a big change all at once, IMO, especially for something where it's nice for people to be able to test it easily. |
@ulfalizer I want to underscore this point. My company has had to develop a fair amount of tooling (with several FTEs involved) just to keep our Zephyr applications working at all given the amount of churn that has gone on (and, to be fair, continues to go on -- the DTS changes around LEDs and GPIO keys are currently preventing us from merging master and are the reason why the latest Zephyr newsletter has not gone out, since I currently write those as part of our merge workflow). If the project is hitting the point where it's generally useful in "enough" scenarios where we can start to worry about stability, I'm 100% in favor of calming down eager merges despite lack of "points of no return" in order to have a more stable mainline. |
@mbolivar The initial version of the PR was a point-of-no-return that removed the C tools. I agree with @SebastianBoe that it's better to do it slowly and incrementally, so I changed it to just add the experimental With the experimental From a practical perspective, I think that's a nicer and more efficient workflow compared to having one huge PR at the end: less rebasing, easier to test, and small and digestible incremental improvements. |
I understood from the TSC meeting that the Windows installation changes would come in the form of an additional Git repository under zephyrproject-rtos on GitHub and inclusion of its build artifacts in PyPI. So the incremental changes to this PR would just be adding documentation (or requirements.txt changes) to pull that package in from PyPI. I don't see why that is "huge". Am I missing something? |
@mbolivar It's not really productive to argue back and forth though, so unless you change your mind, I'll go work on some stuff that will make this PR invalid as-is anyway (because it assumes the Kconfiglib change). :) |
In my view, from a project perspective, encouraging wider testing by merging into mainline is merited only after demonstrating the design's feasibility for use on supported platforms. A good-enough Windows workflow for installing this program is a prerequisite for such a demonstration, especially since curses is just one of multiple potential backends still under discussion, others of which already have been shown to work on Windows with installation via pip.
That's fair and your decision to make. I'm just responding to the code under consideration in this PR here. |
I talked to Cristoph Gohlke, and he's fine with us mirroring the Windows curses wheels and their source code. I created a repository at https://github.com/ulfalizer/windows-curses-wheels for rebuilding the wheels easily. It has an overview of how the wheels work, and doubles as build instructions. In practice, there seems to be more backwards- and forwards-compatibility than that would imply, but I did the "proper" version in case we'd need it. I'm wondering if it's possibly to automatically install a particular wheel depending on what Python version is installed. Otherwise, a simple script that runs |
I'm looking at getting the wheels onto PyPI now. If things work like I suspect they do, it might be enough to just drop all the wheels there, and |
Yep, looks like it's that simple. I built a When I just had a different version of the wheel on PyPI, I could extend the instructions to cover adding the wheels to PyPI as well. Any complaints about |
@mbolivar
Python 2.7, 3.5, 3.6, and 3.7 (32- and 64-bit, for all of them) are supported. Compiling for Python 3.4 is a bit of a pain, as the 64-bit version of Visual C++ 10.0 isn't free, plus installing Visual Studio 2010 when Visual Studio 2017 is already installed is messy (the opposite works better). Python 3.5 was released in September 2015, so maybe we can live without 3.4 for now. The wheels are built from https://github.com/zephyrproject-rtos/windows-curses. The project needed to be renamed anyway, as PyPI isn't too happy about "curses" as a name. :) |
I tried this locally with one of the supported combinations (the one on my computer) which is Python 3.6, 64-bit. Worked like a charm. |
Sounds great! Do you think you could post a diff to the installation documentation as well just so I can understand the flow? I'm not sure if this is a windows-only thing or if it can be added to the 'core' requirements.txt, and there are other open questions about whether we want a scripts/win-requirements.txt if this is Windows-only, etc. Just trying to make sure we've nailed down the impact on users here. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just trying to make sure we've nailed down the impact on users here.
@carlescufi explained to me offline that this is a Windows-only step that will need to be mentioned from the Windows getting started guide, but that should be done (via requirements file or manual pip install) to get the Windows installation to parity with other platforms where these tools are concerned. That answers my questions about how it affects users.
I'm comfortable with that summary and plan for how to proceed. OK for getting the PR in for now, with the proviso that I think we need to nail down the docs (I'm in favor of adding a scripts/win-requirements.txt for this initially) before 1.12.
@mbolivar There's a section that explains how to add support for a new Python version now too, with the patch included. I thought we had a |
Great! That's good for the windows-curses package's documentation, but a first-time Zephyr user should not have to hunt that down when getting started IMO.
Yup -- and setting that up / documenting this in a clean way is the final part of setting up the removal of the C tools, I think. |
This series adds a Kconfiglib-based menuconfig implementation, built
with the standard Python
curses
module. A newpymenuconfig
target isadded to run it.
The C tools are kept for now. Removing them separately makes it possible
to test
pymenuconfig
alongside the C tools, and keeps changes small andfocused.
A feature is planned for later that shows all symbols -- including those
that aren't currently visible -- along with a search and "jump to"
feature. Loading of arbitrary
.config
files will be supported later aswell (as opposed to always loading
.config
/KCONFIG_CONFIG
). Thosefeatures are all connected implementation-wise.
For Windows, the wheels at
https://www.lfd.uci.edu/~gohlke/pythonlibs/#curses provide the curses
implementation. They use the standard Python curses module
(
_cursesmodule.c
), linked against PDCurses.Running
python -VV
gives the Python version and bitness, to know whichwheel to install. User documentation will be added once the C tools are
removed and the
pymenuconfig
target is moved over tomenuconfig
.The CMake parts are originally by @SebastianBoe.
Description, taken from the menuconfig.py docstring:
Building the wheel
(Compiled wheels are already available, but for documentation purposes.)
Unpack
curses‑2.2‑source.zip
from https://www.lfd.uci.edu/~gohlke/pythonlibs/#curses somewhere.Clone PDCurses (https://github.com/wmcbrine/PDCurses) into the same directory.
Build PDCurses by running this command in the
wincon\
directory (the Windows console PDCurses backend):WIDE=y
is needed for Unicode support, whileUTF8=y
forces UTF-8 as the encoding (not essential, but maybe it can avoid obscure Unicode issues).Build the wheel by running this command in the top-level directory: