Skip to content

Fix alert flags so that Merlin understands them#5245

Merged
snowleopard merged 1 commit intoocaml:mainfrom
snowleopard:fix-unstable-alerts
Nov 29, 2021
Merged

Fix alert flags so that Merlin understands them#5245
snowleopard merged 1 commit intoocaml:mainfrom
snowleopard:fix-unstable-alerts

Conversation

@snowleopard
Copy link
Copy Markdown
Collaborator

As discussed on Slack, it seems that Merlin behaves differently from the compiler when interpreting alert flags. As suggested by @voodoos, I'm tweaking the flags to help Merlin a bit. This solves the issue for me locally.

Signed-off-by: Andrey Mokhov <amokhov@janestreet.com>
@snowleopard snowleopard requested review from a user, rgrinberg and voodoos November 29, 2021 16:35
(env
(_
(flags :standard \ -alert=-unstable)))
(flags :standard \ -alert -unstable)))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That'll work for this case, but in general it means: exclude "-alert" and exclude "-unstable" which is not the same as exclude "-alert -unstable". It'll work for us as "-unstable" doesn't appear anywhere else.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, removing flags in this way is unsafe but I guess we don't have a safer mechanism. Hopefully, it'll not bite us in this particular case.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If the intent is to enable the unstable alert, I think you can use :standard -alert +unstable.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Oh, good point, let me try that.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The intent is to remove the flag because configurator needs to build with 4.02 and 4.02 doesn't know the -alert flag.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah right, then it doesn't help, sorry for the noise.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

OK, merging as is then.

Copy link
Copy Markdown

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants