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 #18836 #19663

Merged
Merged

Conversation

ana-borges
Copy link
Contributor

@ana-borges ana-borges commented Sep 28, 2021

Fix for #18836.

CoqIDE also seemed to be affected.

@ana-borges ana-borges marked this pull request as draft September 28, 2021 16:21
@ana-borges ana-borges force-pushed the 18836-An-env-var-affects-the-build-of-coq branch from 3846447 to 5e79035 Compare September 28, 2021 16:29
@ana-borges ana-borges marked this pull request as ready for review September 28, 2021 16:29
@ana-borges ana-borges force-pushed the 18836-An-env-var-affects-the-build-of-coq branch from 5e79035 to 56a397c Compare September 28, 2021 17:45
@ejgallego
Copy link
Contributor

Thanks @ana-borges , the fix should only be needed for Coq 8.10-8.13 tho, let me actually check when this problem was introduced.

Copy link
Contributor

@ejgallego ejgallego 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 the patch @ana-borges , however this fix is only needed for Coq 8.12.x and Coq 8.13.x ; I thus suggest you amend the PR.

@ejgallego
Copy link
Contributor

Also, I'd suggest you update the PR / commit message to fixes #18836 , so Github will link this patch to the issue it is solving

@ana-borges ana-borges changed the title Unset COQ_USE_DUNE during coq build Fixes #18836 Sep 29, 2021
@ana-borges ana-borges changed the title Fixes #18836 Fix #18836 Sep 29, 2021
@ana-borges
Copy link
Contributor Author

this fix is only needed for Coq 8.12.x and Coq 8.13.x ; I thus suggest you amend the PR.

Thank you for checking @ejgallego.

Did you also check CoqIDE? I tried checking again and couldn't install it with or without COQ_USE_DUNE, probably for an unrelated reason (I remember not being able to install last year either). So I may easily have been wrong yesterday when I concluded this problem affected it as well.

@ana-borges ana-borges force-pushed the 18836-An-env-var-affects-the-build-of-coq branch from 56a397c to 3bb53f7 Compare September 29, 2021 15:47
@ejgallego
Copy link
Contributor

ejgallego commented Sep 29, 2021

Thanks @ana-borges , indeed coqide packages need the same fix; well-spotted.

@ana-borges ana-borges force-pushed the 18836-An-env-var-affects-the-build-of-coq branch from 3bb53f7 to 5b7b2e9 Compare September 29, 2021 18:42
@mseri
Copy link
Member

mseri commented Oct 7, 2021

@ejgallego is this ready to merge?

@ana-borges
Copy link
Contributor Author

@mseri: There are a bunch of CI failures, which I guess should be fixed before merging, right?

I failed to reproduce the errors due to not being able to install all dependencies, and then I got busy with other stuff and couldn't pick this up again. Next week I will have more time. If you or anyone has some insight on the errors, I would appreciate it.

@mseri
Copy link
Member

mseri commented Oct 7, 2021

All the failures are related to missing or incorrect system packages, we cannot do anything to solve them. I'll postpone deciding the merge to next week after you had time to have a look. Thanks for the prompt response

Copy link
Contributor

@ejgallego ejgallego left a comment

Choose a reason for hiding this comment

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

Indeed, this looks good, thanks @ana-borges !

@ana-borges
Copy link
Contributor Author

@mseri Ok, I see that these errors were already there. Thank you for pointing this out. Feel free to merge!

@mseri mseri merged commit e9424f1 into ocaml:master Oct 12, 2021
@mseri
Copy link
Member

mseri commented Oct 12, 2021

Thanks both for the help

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

Successfully merging this pull request may close these issues.

3 participants