Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.

Makefiles: Enable -checkaction-context for unittest builds#2836

Merged
wilzbach merged 1 commit intodlang:masterfrom
MoonlightSentinel:enable-checkaction
Nov 7, 2019
Merged

Makefiles: Enable -checkaction-context for unittest builds#2836
wilzbach merged 1 commit intodlang:masterfrom
MoonlightSentinel:enable-checkaction

Conversation

@wilzbach
Copy link
Copy Markdown
Contributor

wilzbach commented Oct 24, 2019

Yeah, I am not sure -checkaction=context is ready for this yet. Anyhow, the error that you're getting is due druntime now being build with -checkaction=context as well and thus build errors:

src/core/lifetime.d(1565):        instantiated from here: `_d_assert_fail!("is", S5, S5)`

@wilzbach
Copy link
Copy Markdown
Contributor

wilzbach commented Oct 24, 2019

BTW @MoonlightSentinel please feel free to send me a ping if you want to be part of the DLang Slack. It's often more efficient to talk there ;-)

@MoonlightSentinel
Copy link
Copy Markdown
Contributor Author

I am not sure -checkaction=context is ready for this yet. Anyhow, the error that you're getting is due druntime now being build with -checkaction=context as well and thus build errors:

src/core/lifetime.d(1565):        instantiated from here: `_d_assert_fail!("is", S5, S5)`

Agreed, but testing this feature on a large codebase (druntime/phobos) seems useful to flush out any remaining bugs (hence the draft PRs)

BTW @MoonlightSentinel send me a ping if you want to be part of the DLang Slack. It's often more efficient to talk there ;-)

@wilzbach Like this? (Sorry, first 'real' GitHub project)

@wilzbach
Copy link
Copy Markdown
Contributor

Agreed, but testing this feature on a large codebase (druntime/phobos) seems useful to flush out any remaining bugs (hence the draft PRs)

👍

@wilzbach Like this? (Sorry, first 'real' GitHub project)

Well, I was more asking for a quick email (sorry for not being clearer). Anyhow, I was just asking for your consent because I didn't want to send things to your email address without asking.
The Slack room is somewhat semi-public. We can't make it fully public because there's a limit on the # of people we can have there, but on the other hand that helps the noise:info ratio compared to the NG.
Anyhow, happy chatting!

@MoonlightSentinel
Copy link
Copy Markdown
Contributor Author

Well, I was more asking for a quick email (sorry for not being clearer). Anyhow, I was just asking for your consent because I didn't want to send things to your email address without asking.

Thanks!

The Slack room is somewhat semi-public. We can't make it fully public because there's a limit on the # of people we can have there, but on the other hand that helps the noise:info ratio compared to the NG.
Anyhow, happy chatting!

Seems reasonable

@MoonlightSentinel MoonlightSentinel force-pushed the enable-checkaction branch 3 times, most recently from 478b89f to 34a1a55 Compare November 1, 2019 22:48
@MoonlightSentinel
Copy link
Copy Markdown
Contributor Author

MoonlightSentinel commented Nov 2, 2019

There are two remaining failures which I cannot reproduce locally but seem to occur reliable on the CI systems:

  • dmd/backend/symbol.d:1170: Assertion '(*s).Ssymnum == -1' failed. [auto-tester (windows) and buildkite]

Verbose output lists function rt.util.container.array.__unittest_L143_C1 as the current task when hitting the assert.

  • ThreadError in test/gc/sigmaskgc.d [Circle CI]
Testing sigmaskgc
./generated/linux/debug/64/sigmaskgc
uncaught exception
core.thread.osthread.ThreadError@src/core/thread/osthread.d(3176): Unable to suspend thread
----------------
...
----------------
uncaught exception
core.thread.osthread.ThreadError@src/core/thread/osthread.d(3176): Unable to suspend thread
----------------
make[2]: Leaving directory '/home/circleci/druntime/test/gc'

Any suggestions?

@MoonlightSentinel MoonlightSentinel marked this pull request as ready for review November 6, 2019 22:47
@MoonlightSentinel
Copy link
Copy Markdown
Contributor Author

Unittest builds using checktion=context pass the test suite on any plattform (without any of the aforementioned errors)!

Copy link
Copy Markdown
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! Thank you very much!

@nordlow
Copy link
Copy Markdown
Contributor

nordlow commented Nov 18, 2019

Keep up this awesome work!

Making -checkaction=context stable and non-disruptive for the remaining corner-cases will become a big selling point for D over most (all) other languages.

@MoonlightSentinel MoonlightSentinel deleted the enable-checkaction branch November 18, 2019 15:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants