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

agda 2.6.1.2 #67014

Closed
wants to merge 3 commits into from
Closed

agda 2.6.1.2 #67014

wants to merge 3 commits into from

Conversation

carlocab
Copy link
Member

@carlocab carlocab commented Dec 16, 2020

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

This doesn't work for me yet locally, but I suspect there might be an issue with my system. Hoping CI will produce something better. Or, at least, more informative errors.

Cf. #65000, #55253, #65831

@carlocab carlocab marked this pull request as draft December 16, 2020 15:32
@BrewTestBot BrewTestBot added the haskell Haskell use is a significant feature of the PR or issue label Dec 16, 2020
@carlocab carlocab force-pushed the agda-2.6.1.2 branch 8 times, most recently from a45f0b3 to d4bee16 Compare December 17, 2020 04:07
@carlocab carlocab force-pushed the agda-2.6.1.2 branch 7 times, most recently from 9178434 to 0cd8ec4 Compare December 17, 2020 08:22
@carlocab carlocab marked this pull request as ready for review December 17, 2020 08:33
@mitchblank
Copy link
Contributor

After this merges try seeing if cedille builds, since it depends_on agda

@carlocab
Copy link
Member Author

carlocab commented Dec 17, 2020

Yes, I've had a look at cedille. It's not going to build; the formula uses bits of brew that have now been disabled. It needs a revamp...

@carlocab
Copy link
Member Author

carlocab commented Dec 17, 2020

Error: agda: failed
An exception occurred within a child process:
  Timeout::Error: execution expired

Ugh. Does anyone know if there's a way to extend this timeout?

@SMillerDev
Copy link
Member

@MikeMcQuaid any DSL for extending the test timeout?

@MikeMcQuaid
Copy link
Member

@SMillerDev Nope, sorry. Probably just warrants the test being simplified.

@carlocab
Copy link
Member Author

Tbf I suspect the test is excessive, and probably could be cut down too. Except I don't really know agda so I'm a bit hesitant to change too much about the test.

@carlocab
Copy link
Member Author

carlocab commented Dec 17, 2020

@MikeMcQuaid Just to understand what I need to do about the test block: does the timeout apply to the entirety of the block, or just to the individual commands that it runs?

@gromgit
Copy link
Member

gromgit commented Dec 17, 2020

Not an agda guru either, but I suspect cutting the test down to the stdlibiotest stuff (a.k.a. "Hello World") should be sufficient.

@carlocab
Copy link
Member Author

carlocab commented Dec 17, 2020

I actually think it's the calls to cabal that are the useless part of the test block, but that might be from my having made calls to it about four dozen times now over the course of fixing this formula.

Update: Ok, guess they weren't so useless.

@carlocab carlocab marked this pull request as draft December 17, 2020 20:43
@carlocab
Copy link
Member Author

carlocab commented Dec 17, 2020

Still working on this. FYI. Yep, should be good to merge if CI is green.

@carlocab carlocab force-pushed the agda-2.6.1.2 branch 2 times, most recently from 64ddef5 to 576b032 Compare December 17, 2020 23:19
It leads to the formula failing CI due to the test timing out.
@carlocab carlocab marked this pull request as ready for review December 18, 2020 00:31
Copy link
Member

@chenrui333 chenrui333 left a comment

Choose a reason for hiding this comment

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

awesome work!! 💯

@chenrui333 chenrui333 added the cabal v1 deprecation Deprecate cabal sandbox label Dec 18, 2020
@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@carlocab carlocab deleted the agda-2.6.1.2 branch December 18, 2020 06:29
@carlocab carlocab mentioned this pull request Dec 18, 2020
carlocab added a commit to carlocab/homebrew-core that referenced this pull request Jan 2, 2021
The versioned dependency conflict was resolved in Homebrew#67014.
Rylan12 pushed a commit that referenced this pull request Jan 2, 2021
The versioned dependency conflict was resolved in #67014.
@mietek
Copy link
Contributor

mietek commented Jan 7, 2022

It would have been nice to ping some of the previous contributors to this formula about the changes to this formula.

The removed test was useful, because it tested that the standard library has been properly installed, which is not trivial to get right, and in fact the formula has been installing a broken standard library in the past.

The removed caveats were useful, because people continue to be confused about the proper way to set up Agda:
https://twitter.com/lindsey/status/1479258217905491968

Why were the caveats removed? Was it just an omission or something else? We could replace the caveats by automatically modifying files in the user’s home directory upon installation, but this has been discouraged in the past.

Why was the compilation of the agda-mode elisp removed?

@carlocab
Copy link
Member Author

carlocab commented Jan 7, 2022

The test was removed because it was failing due to the timeout. (See comments above.) The caveats were an omission. The agda-mode elisp compilation failed when I built this locally.

Pull requests to improve the formula are welcome.

Note that I don't use Agda. (See comments above.) I just picked this up because it had been impossible to build from source with the Agda formula for nearly a year. It would've been ideal for this to have been done by someone who actually uses Agda, but it didn't look like that was going to happen. Maybe you'd like to fix up the changes I made here.

@mietek
Copy link
Contributor

mietek commented Jan 7, 2022

I understand the removed test has started failing at some point due to changes in Homebrew. This seems to be a problem with these changes rather than with the test, which should only take seconds to run locally. How long did it take for you?

Regarding the caveats, what is the current policy on modifying files in the user’s home directory upon installation?

Specifically, can the formula do something like this:

  • check that ~/.agda does not exist;
  • create ~/agda;
  • create ~/.agda/libraries containing #{HOMEBREW_PREFIX}/lib/agda/standard-library.agda-lib;
  • create ~/.agda/defaults containing standard-library.

@carlocab
Copy link
Member Author

carlocab commented Jan 7, 2022

This seems to be a problem with these changes rather than with the test

It seems to be a problem with virtualisation.

How long did it take for you?

I don't recall, but it didn't take long. It was only a problem in CI.

what is the current policy on modifying files in the user’s home directory upon installation?

This is not allowed by the sandbox. Attempting to modify files outside of HOMEBREW_PREFIX or HOMEBREW_TEMP will fail.

@mietek
Copy link
Contributor

mietek commented Jan 7, 2022

Thank you. I expect that the security policy also does not allow persisting an environment variable change to happen automatically upon installation?

Specifically, can the formula set AGDA_DIR to a path inside HOMEBREW_PREFIX so that Agda is always executed with this setting?

@carlocab
Copy link
Member Author

carlocab commented Jan 7, 2022

You can create a wrapper script that sets AGDA_DIR before executing agda using write_env_script or env_script_all_files.

@github-actions github-actions bot added the outdated PR was locked due to age label Feb 7, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cabal v1 deprecation Deprecate cabal sandbox haskell Haskell use is a significant feature of the PR or issue outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants