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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Print a warning when assertions are enabled #8240

Merged
merged 3 commits into from
Jul 2, 2022

Conversation

jneira
Copy link
Member

@jneira jneira commented Jun 21, 2022

I want to remember sometimes cabal builds with assetions enabled were leaked to final users.
Anyways i think warn about it would be useful for the reasons noted in #4377

Not sure how could this be tested without build a cabal version with assertions enabled and not sure it it worths it


Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!

@jneira jneira requested a review from grayjay June 21, 2022 20:29
@Mikolaj
Copy link
Member

Mikolaj commented Jun 22, 2022

Ideally, we'd distribute alpha versions with a subset of assertions enabled (and enable them for dev builds by default, which probably means all builds from repo source --- do we do that already?), but that would require more than DEBUG_EXPENSIVE_ASSERTIONS, because we'd need to exclude nice-to-have assertions and include only OMG-wrong-results-will-now-silently-be-emitted.

@jneira jneira marked this pull request as draft June 22, 2022 21:33
@jneira
Copy link
Member Author

jneira commented Jun 22, 2022

The expensive assertions message is not printed cause the flag and the cpp was moved to cabal-solver after the original pr.
Now i am not sure if it worths to add again them to cabal-install only to be more precise about the message,
Maybe the warning about assertions is enough.
thougts?

@Mikolaj
Copy link
Member

Mikolaj commented Jun 22, 2022

I'd find the more precise warning helpful, but then I'm an assertions aficionado.

@jneira jneira marked this pull request as ready for review June 27, 2022 21:17
@jneira
Copy link
Member Author

jneira commented Jun 27, 2022

Specific warning restored:

PS D:\dev\ws\haskell\cabal> cabal build cabal-install --ghc-options=-fno-ignore-asserts -f+debug-expensive-assertions --disable-tests
....
PS D:\dev\ws\haskell\cabal> iex "$(cabal list-bin cabal-install) --version"
Warning: this is a debug build with expensive assertions enabled. This will negatively affect executable performance.
cabal-install version 3.9
compiled using version 3.9.0.0 of the Cabal library

@jneira
Copy link
Member Author

jneira commented Jun 28, 2022

@grayjay respectful ping, I think you could be interested in

Copy link
Collaborator

@grayjay grayjay left a comment

Choose a reason for hiding this comment

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

Thanks for updating #4552!

Comment on lines 41 to 44
flag debug-expensive-assertions
description: Enable expensive assertions for testing or debugging
default: False
manual: True
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand the purpose of this flag. It looks like it is independent of the one in the solver package, so the warning in this PR doesn't indicate whether the slow functions in the solver are enabled. I don't think that there is a way to make the cabal-install and solver packages share one CPP option, so Main.hs would probably need to call expensiveAssert directly and check for an exception, similarly to how it calls assert.

I also think that it would be fine to leave out the warning about expensive assertions, because they are difficult to enable unintentionally. When I added the flag in #4346, I only intended for it to be enabled in one CI job, in order to check the assertions in tests. I think that the expensive assertions are slow enough that developers wouldn't want to enable them regularly when solving for install plans involving Hackage.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, the cabal flag must have been split when cabal-install and cabal-install-solver had been split. I guess we (may in the future) use the flag in both, but we should treat them separately, alas.

Copy link
Member

Choose a reason for hiding this comment

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

But, as @jneira remarks, using the flag and warning about the flag is a different thing, so I'm OK with simplifying.

description: {

- Now cabal-install executable will print a warning if assertions are enabled
- The message will mention specifically the us of expensive assertions (currently present only in Cabal-solver)
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/us/use/

@jneira
Copy link
Member Author

jneira commented Jun 29, 2022

hmm I thought if the flag name matches all local packages would be parametrized by
but agree the pr will be cleaner without the expensive part so if you think it does not worth it I will remove

@grayjay
Copy link
Collaborator

grayjay commented Jun 30, 2022

hmm I thought if the flag name matches all local packages would be parametrized by

The flags can be specified independently, as in cabal build cabal --constraint=cabal-install+debug-expensive-assertions.

but agree the pr will be cleaner without the expensive part so if you think it does not worth it I will remove

I think it is much more important to warn about enabling regular assertions, because they are useful in more situations, and it is easier to enable them accidentally. Additionally, even if you revert the part that handles expensive assertions, there will still be a warning whenever they are enabled, since they also require regular assertions to be enabled.

@jneira jneira requested review from Mikolaj and grayjay July 1, 2022 11:41
@jneira
Copy link
Member Author

jneira commented Jul 1, 2022

@Mikolaj @grayjay As discussed, i've restored the version which only warns about assertions enabled

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

This is fine.

Copy link
Collaborator

@grayjay grayjay left a comment

Choose a reason for hiding this comment

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

Thanks!

@jneira jneira merged commit 0c4d048 into haskell:master Jul 2, 2022
@andreasabel
Copy link
Member

Warning updated by

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-backport in the future e.g., to a point release after the main release attention: needs-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Print a warning when cabal is built with assertions enabled
5 participants