Skip to content

"Build profile" now reflects optimization level set in global config #8488

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

Merged
merged 1 commit into from
Oct 1, 2022

Conversation

ulysses4ever
Copy link
Collaborator

@ulysses4ever ulysses4ever commented Sep 20, 2022

It still reports local config's optimization level if it was set.

Fix #8487.


Please include the following checklist in your PR:

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

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

Looks great, a good ux improvement with minimal code
I would add a regression test, i hope it will not be too difficult. And the usual changelog entry

@ulysses4ever ulysses4ever force-pushed the t8487-build-profile-msg-reflect-global-config branch 3 times, most recently from 28f3401 to 41fb733 Compare September 21, 2022 02:04
@ulysses4ever
Copy link
Collaborator Author

@jneira thanks for the feedback! Test and changelog added.

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

lgtm, thanks

@Mikolaj
Copy link
Member

Mikolaj commented Sep 24, 2022

@mouse07410: could you kindly review, given that this PR is based on your input?

@ulysses4ever
Copy link
Collaborator Author

ulysses4ever commented Sep 24, 2022

@mouse07410 To test locally, it may be sufficient to download the cabal binary built by the CI run for this PR. At the very bottom of this page you can find the binaries for the three main platforms.

@mouse07410
Copy link
Collaborator

I confirm that cabal-macOS-9.2.3 works as expected (within my limited tests) on MacOS Monterey with GHC-9.4.2, displaying the correct "Build profile":

$ cabal-macOS-9.2.3 build
Warning: this is a debug build of cabal-install with assertions enabled.
Resolving dependencies...
Build profile: -w ghc-9.4.2 -O2
In order, the following will be built (use -v for more details):
 - stepic-tests-0.1.0.0 (lib) (first run)
.  .  .

I tried to post a review, but couldn't figure how to do it - it does not seem to let me, even after I joined the "cabal" organization (no pun intended ;)).

Copy link
Collaborator

@mouse07410 mouse07410 left a comment

Choose a reason for hiding this comment

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

Tested on my Mac (with GHC-9.4.2), works as expected. Note: my testing was pretty limited.

@ulysses4ever
Copy link
Collaborator Author

@mouse07410 thank you!

@Mikolaj do you know what’s up with permissions for @mouse07410? In the list of approvals for this PR, I see a gray check mark for them and a green one for jneira, and CI says that there’s only one approval…

@Mikolaj
Copy link
Member

Mikolaj commented Sep 27, 2022

@Mikolaj do you know what’s up with permissions for @mouse07410? In the list of approvals for this PR, I see a gray check mark for them and a green one for jneira, and CI says that there’s only one approval…

It must be what the Triage permission level enables, the one I was advised to grant by default unless a higher level is explicitly required. That's quite unfortunate...

@mouse07410 mouse07410 self-requested a review September 27, 2022 20:15
@Mikolaj
Copy link
Member

Mikolaj commented Sep 27, 2022

@mouse07410, I've promoted you to authority level WRITE. Use your new-found powers well. :)

@ulysses4ever ulysses4ever added merge me Tell Mergify Bot to merge and removed attention: needs-review labels Sep 27, 2022
@mouse07410
Copy link
Collaborator

I've promoted you to authority level WRITE. Use your new-found powers well. :)

Thank you!

I think I need to do a lot of READ before I can contemplate WRITE. ;-)

@mouse07410
Copy link
Collaborator

I did a couple more tests - specifically, explicit settings in ~/.cabal/config and $PROJECT_HOME/cabal.project.local. Worked as expected (correctly).

I don't recall if optimization: N has to go to cabal.project or cabal.project.local - or it's possible to define in <project>.cabal file?

@Mikolaj
Copy link
Member

Mikolaj commented Sep 28, 2022

I don't recall if optimization: N has to go to cabal.project or cabal.project.local - or it's possible to define in <project>.cabal file?

At least optimization: False is legal in cabal.project and cabal.project.local (I haven't tried a number there). I set it in <project>.cabal via ghc-options: -O2, so I'm not sure if it can be done more natively there.

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 30, 2022
@ulysses4ever
Copy link
Collaborator Author

@mergify rebase

It still reports local config's optimization level if it was set.

Fix #8487.
@mergify
Copy link
Contributor

mergify bot commented Oct 1, 2022

rebase

✅ Branch has been successfully rebased

@andreabedini andreabedini force-pushed the t8487-build-profile-msg-reflect-global-config branch from 41fb733 to f33e5e9 Compare October 1, 2022 12:49
@mergify mergify bot merged commit a5106be into master Oct 1, 2022
@andreabedini andreabedini deleted the t8487-build-profile-msg-reflect-global-config branch December 14, 2023 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Build profile" message doesn't reflect optimization setting from ~/.cabal/config
4 participants