-
Notifications
You must be signed in to change notification settings - Fork 213
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
Restructure flag inheritance in platformio. #500
base: main
Are you sure you want to change the base?
Conversation
I'm tired of watching huuuuuge lists of files build that I know won't be used. I've not shaved everything to the bone, but I've cut the cases that are big enough to annoy me out of the builds I care about. For example, waiting for it to check out and build that ridiculous olikraus/U8g2 library build up to 37 times on a full build is a drag. Anything that used to set USE_SCREEN now uses ${use_screen.build_flags} to get -DUSE_SCREEN and ${parent-device.lib_flags} to get the dependencies and the -L ... -l stuff. I similarly tried to tighten up the remote_flags case, upon which this was inspired. There's still a bit of silliness, such as everything checking out and building BUTTONS when almost none of our builds HAVE buttons...but that lib builds quickly enough to not annoy me. Now checking it out and storing it 38 times isn't awesome. The way we have configuration split between globals.h and platform.ini is definitely sub-awesome. I wish I could figure out how to make Platformio's extra_script.py to look at the -D flags in CPPFLAGS and then add_libs based on that. Then when globals.h consitionally adds a -DFoo, we could add_libs Foo without it having to appear in globals.h It's pretty silly to wait and checkout and build AsyncTCP or WebServer if the configuration didn't even turn on ENABLE_WIFI, but we do If your build is broken, it's probably this PR's fault. Buzz me with a repro case for help if needed. Notably, this tightens many environments to use the "correct" hardware lib_deps (which may include extra libraries as it might have been extended) instead of those of base_libs. Tested: Successful python3 tools/build-all.py
@robertlipe Thanks for submitting this. The segmented inheritance within platformio.ini is totally in line with the pattern I implemented in a massive overhaul of platformio.ini some time ago, so I obviously can't disagree with that. (That exercise aimed to significantly decrease repetition in the env blocks, not to decrease the inclusion of unnecessary dependencies.) I've given the specific change some thought, and I'd like to propose a few small changes:
Let me know your thoughts on this. |
I suspect we (and anyone else that stares at it for very long) agree that our platformio is sub-awesome. I get wanting to use preprocessor for everything, but it clutters the namespace and I've tried to make https://docs.platformio.org/en/latest/scripting/index.html (and related) look for (handwaving) -DWANT_FOO in order to add lib_deps += libfoo; I just can't figure out how to make that work well. IMO, we should drive MUCH more configuration into the platform.ini (ick) where it's in build space and can be fondled and monitored by python and scons and out of preprocessor-land. But that's not the total point of this. Sure, I can do a global search and replace of USE_SCREEN with HAS_SCREEN. It didn't really meet Blinkenbit guideline, but I don't hate adding that onto the load here. I slightly prefer the active verb form we have now (just because we HAVE a screen doesn't mean we want to USE that screen) but I really won't dig in my heels on that. I'll submit a global search and replace in a bit. The more I dig into our REMOTE handling, the more screwy I think it is. globals.h actually turns it ON unless it's turned OFF elsewhere. NightDriverStrip/include/globals.h Line 1391 in b3f87ba
NightDriverStrip/include/globals.h Line 1213 in b3f87ba
The result is that remote is MOSTLY included in all the builds (and I hate, hate, hate waiting for it to check out and build dozens of 900KB files that are effectively empty) when the reality is that our ENABLE_REMOTE flags - especially after this change, mostly turns ON the -DDONT_COMPILE_THE_ENTIRE_PLANET flag, meaning that it's the builds that don't ENABLE_REMOTE or set remote_flags AT ALL that get penalized most by compiling dozens of those 900KB files that are basically #if 0'ed away. But that's another scab I was reluctant to pick too deeply at. So I thought about it and really don't have a great plan for that last one. On one hand, we want plat*ini to use remote_flags for the lib_deps because otherwise we end up sucking in and compiling that giant remote library that we may or may not want. OTOH, we want -DENABLE_REMOTE to be lightweight in special developer pet projects (ala NightDriverStrip/include/globals.h Line 347 in b3f87ba
So I ack point 1 and agree that a rename to SUPPORTS_SCREEN, while not my favorite, is a reasonable additional request for this PR and will push it soon. If I fall asleep before you QA and merge this, I'll agree to come back and "fix" that. But then, we should similarly think about USE_HUB75, USE_OLED, USE_M5DISPLAY, USE_TFTPSPI, and a bunch of others, too. My custom board (see external communication on the same) might be a devkit-c that I've attached a 1306 (USE_OLED, see related pins) and want an effect of either SPECTRUM or MESMERIZER. Requiring FOUR configurations seems overkill. I acknowledge point 2 is a problem, but it's deeper than a superficial one. The push and pull between platini and globh is intense and I don't know how to find a happy place for it. It's self-evident that copy-pasting blocks in glob*h is easy (too easy) but that it's a problem just because we have so many internally inconsistent projects. For example, the effect SPECTRUM is jumping through #ifdef hoops to try to be happy with multiple host boards like WROVER and ELECROW and such ... when PIN0 should be a trait of a host board (an env or probably even a dev) and definitely not an effect like "spectrum". REALLY, we want remote_foo in platformio.ini. Otherwise, we have to pull in - and thus, build - the library everywhere. This is how we end up with out builds checking out and building 38 copies of the remote library. OTOH, I totally understand the tempting precision of adding another #ifdef block in *globals.h that precisely defines pins, libraries needed (danger!) and such. Since our remote_flags is the thing that turns OFF the -DCOMPILE_THE_WORLD options and most of our targets end up neededing REMOTE_FLAGS to build, there's a perverse incentive that it's the project that absolutely declared NO interest in needing a remote that get swept into the defaults and thus building with a lib_deps of remote_flags, but not getting the build_flags of remote_flags. This patch makes that better, but it's hard to mentally disprove all the cases that are still pulling in the giant remote builds. I think this change is a solid step forward. Can I push back on #2 and agree to keep thinking about it until we have a solid plan forward that lets us disentangle the platform.ini and the globals.h representations? It's messy. (If I'm misunderstanding, you're free to type more slowly until I see an easy way that actually makes it simpler and not more complicated.) I'll absolutely admit that I keep flying into the gravity of our configuration mess (sorry, not sorry) and alternately thinking that I can do better or getting sucked into the sun and burning. I've really struggled with supporting the "Soldering Iron Crowd" and the normies and trying to find configurations that work for both and haven't yet found a happy place. |
I'm sorry but no, not if you do want to apply selective inclusion of the remote control library dependency in this PR. The reason is this: we now remove the ability to use the remote by not including the necessary library dependency for certain configurations, and that will cause weird problems when people then set (or leave) My statement is basically this: if we selectively include library dependencies for certain capabilities in platformio.ini, we have to tell the code that's being built what we have, and thereby what we don't. That's what the HAS/SUPPORTS (I understand you prefer the latter, which is fine with me) defines are for. That's also where they (should) differ from the USE variety - those should indicate what is chosen to be used. I can definitely imagine that other defines (USE or otherwise) should also be converted to HAS/SUPPORTS, but that can indeed be a follow-up. In a sense, selectively including (or excluding) lib_deps adds another layer of complexity to the multiple levels that are already embedded in the platformio.ini/globals.h combo. I could have raised that as an objection to this change as a whole, but I didn't as I do agree this is a sensible step to take. We just can't abandon the rest of the setup until we fix it, regardless of how icky we think it now looks. So, as far as I see it, we can do one of three things:
|
I'll add my two cents. Thanks for cleaning this up. I can guarantee this change will break my setups, but not sure how. I needed to restart with a fresh fork anyway. I do have reservations about changing ENABLE_REMOTE to SUPPORTS_REMOTE. It is an input feature I want to enable, just like the web server. It is not natively supported by most boards since it requires adding an additional piece of hardware (IR diode). Logically, supports_remote is not true for most boards. ENABLE_REMOTE makes more sense to me. It is a small point in the midst of the greater issue being discussed above. But, it should not be enabled by default. I can appreciate the need for these modifications. Wrapping my brain around the interplay between globals.h and platformio.ini took a while. There is a lot of redundancy and inconsistency in what features are enabled where. Sometimes web server is enabled in plat*.ini and sometimes in globals.h. |
I've been trying to come up with a way forward on this and haven't yet
succeeded. Adding MORE flags is the opposite of a goal for me. Removing
nonsensical components from the build (and that agonizing fetch) is.
Fetching and building that UG8 library for mesmerizer, for example, is a
total waste of electrons.
I agree, Joe, that having it in a flag in global namespace but that is
meaningfully examined in only three source files (which could be lowered)
but thus requires a recompilation of the world on change isn't great. But,
like UG8, these libraries are so darned large that building them - and
hopefully throwing them away - isn't great, either.
The current state where remote is rarely used, included in all builds, but
only sometimes paired with the flag that makes the build less annoying is
just super weird.
One of the tensions (that I've been pulling at for most of my duration
here) is that we effectively have build systems within a build system. We
ALREADY have cases where the flags are out of sync. Moving that for any
project not represented in the build - and I spent the time to ensure that
all included builds worked - from a runtime cost to a build failure
actually seems like a direction forward to me. I would rather move this
forward and move ALL the remote flags, including the one that sets the pin
for those boards that ship with IR sensors, into platformio.ini and out of
globals.h totally. I'm pretty much out of love with any hardware details
inside globals.h.
We already have IR_REMOTE_PIN, and ENABLE_REMOTE. Adding a HAS_REMOTE
doesn't help our support problem. If anything, I'd vote to remove the
defaultl IR_REMOTE_PIN 25 condition which turns it on for everyone, making
IR_REMOTE_PIN && !define ENABLE_REMOTE a hard error that's tested and
enforced quickly, and move the IR_REMOTE_PIN to the board configuration
definitions to the DEVICE definitions in the cases where the pin is known
because it ships on the hardware.
For those people taking bare modules and building them up, they know they
have to build a device configuration that describes their device,
turning on ENABLE_REMOTE and USE_PIN. But I don't have the dozen boards in
question and am sure I'd take the fall for messing up one of them. After
all, I think if you're taking a bare board (I think you're with me in that
category, Joe) and wiring up sensors and maybe a display and maybe buttons
and such, you're building a new DEVICE definition and should be prepared to
provide an accurate definition of your pin configurations (including things
like [ABCDE_PIN]) there instead of having it sprinked between MatrixConfig
and globals and having to #undef this or that.
Maybe we remove the default for IR_REMOTE_PIN and use the presence of THAT
as our global. Then we'd at least have one knob instead of thee.
Building the world and hoping the linker can sort it out at the same time
you're struggling with memory consumption isn't a great situation to be in.
Like web server, it's tempting to build it in and make it checkbox enabled
in the GUI, but there's a substantial code size/build time cost. (I'm also
against building ESPAsyncWebServer in every configuration, even when
ENABLE_WEBSERVER isn't present, but unlike these two, it builds quickly
enough that I don't rage against it when it builds. MOST of lib_deps is not
used in MOST of our actual builds. I just didn't want to try to tackle them
all at once and from the reaction so far, it's a good thing. So we may keep
building QRCode in demo and other zany combinations.
Unless I can come up with some approach that doesn't add to the number of
flags instead of lowering them and overall improving the consistency, I
probably won't move forward with it. It'll probably stay in my private tree
which might just become a permanent fork as I find the saved compilation
time totally worth it. I'm just not happy with the direction this is being
asked to go. I'm still kicking this around but I won't proceed with it
until we can agree on a way forward that I think is actually forward.
I think few of us are happy with the current situation. None of us yet
have produced a clear step forward. We have things that are clearly effects
settings, some that are clearly hardware settings, some that are user
preferences, some that are vestigial (I just noticed that MILLIS_PER_FRAME
is never defined to NOT be '0' - which doesn't seem good), some that are
project settings, etc. all mixed together. If you wanted to build an
UMBRELLA (display) on an ESP32-S3 (psram with appropriate flags), for
example, I think you'll spend as much time #undefing or just changing
existing flags as you will wiring it up. For example, ONBOARD_LED_foo will
be wrong if your ONBOARD_LED is the commonly provided WS2812 and it'll mess
with hardware pins that you might not be expecting to be taken. The
SPECTRUM (display) makes even more greedy assumptions about the hardware
layout.
I keep thinking about this a LOT as I tend to help the DIY crowd a lot. (I
know there are others on both axes there...) It's really messy and so far
we've not found a clearly better way forward. That's why I've been chipping
at making it a little more consistent and a little less annoying in small
steps.
On this PR, I may fail, but this change is useful enough that I may keep
something like it in my work trees going forward.
…On Sat, Nov 18, 2023 at 9:17 PM Joe Schneider ***@***.***> wrote:
I'll add my two cents. Thanks for cleaning this up. I can guarantee this
change will break my setups, but not sure how. I needed to restart with a
fresh fork anyway.
I do have reservations about changing ENABLE_REMOTE to SUPPORTS_REMOTE. It
is an input feature I want to enable, just like the web server. It is not
natively supported by most boards since it requires adding an additional
piece of hardware (IR diode). Logically, supports_remote is not true for
most boards. ENABLE_REMOTE makes more sense to me. It is a small point in
the midst of the greater issue being discussed above. But, it should not be
enabled by default.
I can appreciate the need for these modifications. Wrapping my brain
around the interplay between globals.h and platformio.ini took a while.
There is a lot of redundancy and inconsistency in what features are enabled
where. Sometimes web server is enabled in plat*.ini and sometimes in
globals.h.
—
Reply to this email directly, view it on GitHub
<#500 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCSD377AQOLCOA3KH5MA3DYFF23PAVCNFSM6AAAAAA7HOD5ZWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJXG4ZTCMRYHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I haven't given up on this. Can we remove the conditional compilation completely and EITHER build/link the huge libraries OR link against an empty stub that announces "IR support unavailable" to the console (which I readily conceed we don't always have, but as a developer product, we rely on anyway) on boot? Then the build options are all consistently controlled in the build file instead of split between global.h, -D compile lines, and libraries added at build time? In platform.ini, every target that we define could use either ${active_remote_flags.build_flags} or ${stub_remote_flags.build_flags} Same basic principle for ug8, dev_m5stack, and any other external library that's big enough to be annoying. (HP doesn't accidentally send most of us the world's fastest PC. :-) If the naming of "lib_deps" is too literal, we could create "build_options" that calls out things like IR, Screen, etc. that's basically a copy/pasted/tweaked block of true/false values that ultimately gets sucked into lib_deps. I can probably expose it to tools/show_features somehow for convenience of binary download server-type applications. ug2 and irremote are too big to keep compiling and hope they get optimized away. I'm focusing on them first, but I'd like to find a foundation we can use for most (many?) of our knobs to get them out of globals.h. |
Bluntly put: no. If we want to clean up the compile/build of the overall project, then that clean-up needs to be clean. That means at least:
In my view, the aim should not be to save time on building dependencies. Instead, the aim should be to improve the structure of the project's build-time configuration including dependencies so that it is obvious to a reasonable extent that a) certain dependencies don't need to be built and therefore aren't, and b) they're not there when they could be. |
I'm tired of watching huuuuuge lists of files build that I know won't be
used. I've not shaved everything to the bone, but I've cut the cases
that are big enough to annoy me out of the builds I care about. For
example, waiting for it to check out and build that ridiculous
olikraus/U8g2 library build up to 37 times on a full build is a drag.
Anything that used to set USE_SCREEN now uses ${use_screen.build_flags}
to get -DUSE_SCREEN and ${parent-device.lib_flags} to get the
dependencies and the -L ... -l stuff. I similarly tried to tighten up
the remote_flags case, upon which this was inspired.
There's still a bit of silliness, such as everything checking out and
building BUTTONS when almost none of our builds HAVE buttons...but that
lib builds quickly enough to not annoy me. Now checking it out and
storing it 38 times isn't awesome.
The way we have configuration split between globals.h and platform.ini
is definitely sub-awesome. I wish I could figure out how to make
Platformio's extra_script.py to look at the -D flags in CPPFLAGS and
then add_libs based on that. Then when globals.h consitionally adds
a -DFoo, we could add_libs Foo without it having to appear in globals.h
It's pretty silly to wait and checkout and build AsyncTCP or WebServer
if the configuration didn't even turn on ENABLE_WIFI, but we do
If your build is broken, it's probably this PR's fault. Buzz me with a
repro case for help if needed. Notably, this tightens many environments
to use the "correct" hardware lib_deps (which may include extra
libraries as it might have been extended) instead of those of base_libs.
Tested:
Successful python3 tools/build-all.py
Description
Contributing requirements
main
as the target branch.