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.

Sign up for GitHub

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

Jump to bottom

Fix ./configure --disable-instrumented-runtime never disabling the instrumented runtime #11611

Merged
merged 2 commits into from
Oct 21, 2022
Merged

Fix ./configure --disable-instrumented-runtime never disabling the instrumented runtime #11611

merged 2 commits into from
Oct 21, 2022

Conversation

kit-ty-kate
Copy link
Member

Calling the configure script with --disable-instrumented-runtime would never actually disable the instrumented runtime.
This should also be backported to the 5.0 branch.

@kit-ty-kate kit-ty-kate force-pushed the fix-disable-instrumented-runtime branch from 0e5af24 to 257dff1 Compare October 7, 2022 13:17
@dra27
Copy link
Member

dra27 commented Oct 7, 2022

Nice catch! Slightly "simpler" would be to revert the mistake from 78463a7 which incorrectly changed instrumented_runtime=false to instrumented_runtime=true on L66 of configure.ac (which then makes those bits of configure.ac identical between 4.14 and 5.0/trunk branches).

@dra27 dra27 added this to the 5.0 milestone Oct 7, 2022
@kit-ty-kate
Copy link
Member Author

@Engil do you have a preference whether it should be enabled or disabled by default? If everyone is fine disabling it by default i'll change the PR

@dra27
Copy link
Member

dra27 commented Oct 13, 2022

The instrumented runtime is enabled by default in 4.14 - not sure that should change (especially as it’s then mildly tricky to rebuild the compiler with it in opam)

@damiendoligez
Copy link
Member

The instrumented runtime is enabled by default in 4.14 - not sure that should change

Indeed, that shouldn't be changed in a bugfix branch.

Octachron
Octachron approved these changes Oct 21, 2022
View reviewed changes
Copy link
Member

@Octachron Octachron left a comment

Choose a reason for hiding this comment

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

A agree with the simplified fix.

@dra27 dra27 merged commit be1e1d6 into ocaml:trunk Oct 21, 2022
dra27 pushed a commit that referenced this pull request Oct 21, 2022
@kit-ty-kate @dra27
Fix ./configure --disable-instrumented-runtime never disabling the in…
27d5629 
…strumented runtime (#11611)

Co-authored-by: David Allsopp <david.allsopp@metastack.com>
(cherry picked from commit be1e1d6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Octachron Octachron Octachron approved these changes

Assignees
No one assigned
Labels
no-change-entry-needed
Projects
None yet
Milestone
5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@kit-ty-kate @dra27 @damiendoligez @Octachron