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

Fix repeated with-rtsopts option #4118

Merged
merged 4 commits into from
Jul 3, 2024
Merged

Fix repeated with-rtsopts option #4118

merged 4 commits into from
Jul 3, 2024

Conversation

pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Jul 2, 2024

GHC does not support repeated --with-rtsopts options, and it simply applies the last one. This means many of the baked-in options were actually not being passed, including -N for some of the services and -T for cannon.

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

GHC does not support repeated `--with-rtsopts` options, and it simply
applies the last one. This means many of the baked-in options were
actually not being passed, including `-N` for some of the services and
`-T` for cannon.
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Jul 2, 2024
@MangoIV MangoIV marked this pull request as ready for review July 2, 2024 11:58
Copy link
Contributor

@MangoIV MangoIV left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 208 to 209
-threaded "-with-rtsopts=-N1 -T" -rtsopts -Wredundant-constraints
-Wunused-packages
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why we specify -N1 here:

  1. Its the default anyway
  2. What's the reason behind this, perhaps would be good to document. Seems to have been added without any specified reason: 47684fe

Copy link
Contributor

Choose a reason for hiding this comment

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

seems like it had been N1 as a default when history started in 2017.

@MangoIV MangoIV merged commit 4cf6310 into develop Jul 3, 2024
9 of 10 checks passed
@MangoIV MangoIV deleted the pcapriotti/rtsopts branch July 3, 2024 08:43
@echoes-hq echoes-hq bot added the echoes: technical-roadmap/technical-debt More specific category, to highlight Technical Debt being tackled. label Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: technical-roadmap/technical-debt More specific category, to highlight Technical Debt being tackled. ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants