Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: remove debug builds #21016

Merged
merged 2 commits into from Nov 7, 2019

Conversation

@MarshallOfSound
Copy link
Member

MarshallOfSound commented Nov 7, 2019

As discussed in the releases WG meeting this PR removes the debug builds from our CI and docs. The documentation is replaced with the testing build.

It was concluded that the debug build served no helpful purpose but was causing a maintenance burden and slowing down our CI runs. This frees up resources both in CI and in terms of engineering effort to keep the debug builds passing.

The new recommended way of building Electron locally is the testing.gn config 馃憤

Notes: no-notes

@MarshallOfSound MarshallOfSound force-pushed the no-debug-builds branch from 7ea72a9 to 577af87 Nov 7, 2019
@deepak1556 deepak1556 requested a review from electron/wg-upgrades Nov 7, 2019
Copy link
Member

deepak1556 left a comment

I am good with this change, since I have never used debug builds ever since testing builds and sccache was available.

docs/development/debugging-instructions-macos.md Outdated Show resolved Hide resolved
Co-Authored-By: Robo <hop2deep@gmail.com>
Copy link
Member

nornagon left a comment

I'm concerned about this for a couple reasons:

  1. Debug builds are the only way to get something that's reasonably steppable in a debugger like lldb / gdb. Testing builds are optimized so there's a ton of inlined code that makes it really hard to step through in a debugger. Further, on Linux, we don't currently produce debug symbols for testing builds (see #18676), so there's no way to debug a Linux testing build from CI at all.
  2. Debug builds have faster compile time, faster link time and thus faster total rebuild time locally, and I think that's worth keeping.
  3. Debug builds help us find dependency issues. Since we're not running gn check and other linting steps on our whole codebase, we often miss adding dependencies where they're required. The debug build helps us find those, where without it we might discover those missing deps much later and at some significant pain.

I'm not necessarily against removing the debug builds despite the above, but I wanted to speak in favor of them before the decision is made.

@MarshallOfSound

This comment has been minimized.

Copy link
Member Author

MarshallOfSound commented Nov 7, 2019

@nornagon This is no way prevents you from changing your symbol level locally, or enabling is_component_build locally if you want.

Debug builds are the only way to get something that's reasonably steppable in a debugger like lldb / gdb.

Anecdotal evidence, I've been using testing for the better part of a year and have always been able to use lldb to step through things.

Debug builds have faster compile time, faster link time and thus faster total rebuild time locally

This is provably wrong, our debug CI is slower than testing CI on the same infrastructure consistently.

Debug builds help us find dependency issues. Since we're not running gn check and other linting steps on our whole codebase

We are running gn check on our codebase? It's in our CI pipeline for both linux and macOS.

I believe I outlined the reasons for removing these builds in my original post up the top and as outlined in this comment, if you want to have component builds or change the symbol level locally then you can do that still.

@nornagon

This comment has been minimized.

Copy link
Member

nornagon commented Nov 7, 2019

@nornagon This is no way prevents you from changing your symbol level locally, or enabling is_component_build locally if you want.

It does if doing so causes FTBFS because we don't test it in CI :)

Debug builds are the only way to get something that's reasonably steppable in a debugger like lldb / gdb.

Anecdotal evidence, I've been using testing for the better part of a year and have always been able to use lldb to step through things.

I was doing this yesterday and had serious trouble with it.

Debug builds have faster compile time, faster link time and thus faster total rebuild time locally

This is provably wrong, our debug CI is slower than testing CI on the same infrastructure consistently.

I'm talking about rebuild time, not build-from-scratch time.

Debug builds help us find dependency issues. Since we're not running gn check and other linting steps on our whole codebase

We are running gn check on our codebase? It's in our CI pipeline for both linux and macOS.

Only on some parts of it! We also don't run checkdeps, or any of Chromium's presubmit stuff.

I believe I outlined the reasons for removing these builds in my original post up the top and as outlined in this comment, if you want to have component builds or change the symbol level locally then you can do that still.

Only if it doesn't FTBFS :)

@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented Nov 7, 2019

I think part of what @nornagon is saying is that since testing builds one big executable, it would cause us to miss potentially faulty or missing exports caught by debug.

For example, we have a patch to expose node::Abort and node::Assert; removing this patch raises no suspicions in testing but is a FTBFS in debug. I would want to ensure that we'd somehow be able to see these arguably important signals after removing debug builds.

@nornagon

This comment has been minimized.

Copy link
Member

nornagon commented Nov 7, 2019

After discussion with @MarshallOfSound, I think it's an acceptable trade-off to deprecate support for the component build, as you can still set is_debug = true to get debug symbols and -O0 for debuggability. We'd lose the ability to build a component build鈥攁s Shelley said, we occasionally need to expose additional things there鈥攂ut I don't think we have a compelling use case for the component build itself.

@deepak1556

This comment has been minimized.

Copy link
Member

deepak1556 commented Nov 7, 2019

it would cause us to miss potentially faulty or missing exports caught by debug.

why do we want to care about this when this configuration is not used by us (if we do in future i mean).

@MarshallOfSound MarshallOfSound merged commit b06a479 into master Nov 7, 2019
14 of 15 checks passed
14 of 15 checks passed
appveyor: win-x64-testing AppVeyor build failed
Details
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-woa-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20191107.13 succeeded
Details
electron-arm64-testing Build #20191107.13 succeeded
Details
electron-woa-testing Build #20191107.12 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Nov 7, 2019

No Release Notes

@MarshallOfSound MarshallOfSound deleted the no-debug-builds branch Nov 7, 2019
@deepak1556

This comment has been minimized.

Copy link
Member

deepak1556 commented Nov 7, 2019

Debug builds can cause a lot of pain on windows especially with template parsing ambiguousness in MSSTL and libc++, check the issue with /Zc:twoPhase. We would always welcome patches fixing debug builds, we just don't want to officially support them.

@miniak

This comment has been minimized.

Copy link
Contributor

miniak commented Nov 8, 2019

@deepak1556 this is already removing patches necessary for debug builds :) #21044

@deepak1556

This comment has been minimized.

Copy link
Member

deepak1556 commented Nov 8, 2019

Ah didn't see that, I take back my earlier comment. @codebytere did we upstream those patches ?

@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented Nov 8, 2019

@deepak1556 we don't directly depend on any functionality in them, so i removed them in agreement with:

why do we want to care about this when this configuration is not used by us

in that i don't think we want to care about this haha

@deepak1556

This comment has been minimized.

Copy link
Member

deepak1556 commented Nov 8, 2019

Yeah thats just my personal opinion, since i have never used debug builds. But just to clarify, if we get PRs to fix build issue with debug config, what is our stand ? do we encourage them to use testing instead, because whatever PR that comes in will be a patch. Probably question for the upgrades WG.

@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented Nov 8, 2019

cc @electron/wg-upgrades, shall we add this to an agenda?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can鈥檛 perform that action at this time.