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

emacs 27.1 #59448

Closed
wants to merge 1 commit into from
Closed

emacs 27.1 #59448

wants to merge 1 commit into from

Conversation

CamJN
Copy link
Contributor

@CamJN CamJN commented Aug 11, 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>)?

there were issues with the audit, listed below, not sure how to fix them:

emacs:
  * Formula emacs contains deprecated SPDX licenses: ["GPL-3.0"].
  * Files were found with references to the Homebrew shims directory.
    The offending files are:
      libexec/emacs/27.1/x86_64-apple-darwin19.6.0/emacs.pdmp

@BrewTestBot BrewTestBot added the deprecated license Formula uses a deprecated SPDX license which should be updated label Aug 11, 2020
@miccal
Copy link
Member

miccal commented Aug 11, 2020

I can answer one of your questions:

* Formula emacs contains deprecated SPDX licenses: ["GPL-3.0"].

The license should be:

license "GPL-3.0-or-later"

@chenrui333 chenrui333 removed the deprecated license Formula uses a deprecated SPDX license which should be updated label Aug 11, 2020
@aconchillo
Copy link
Contributor

Seems to be affected by #58984.

@CamJN
Copy link
Contributor Author

CamJN commented Aug 14, 2020

@aconchillo can you elaborate how so? I looked at the issue you linked and it looks like a Haskell sandbox issue?

@aconchillo
Copy link
Contributor

Yes, there's been other packages unrelated to Haskell AFAICT with the same issue. For example: #58683. I don't know how agda has nothing to do with all this, but I'm experiencing the same issue in this PR #59490.

@gromgit
Copy link
Member

gromgit commented Aug 15, 2020

Version-bumping any formula requires testing all the formulae that depend on it:

$ brew uses --include-build --recursive emacs
agda                   cask                   cedille                doxymacs               eless                  emacs-dracula          gforth                 lfe                    mu                     nesc                   proof-general          rtags

@aconchillo
Copy link
Contributor

Thanks @gromgit. That makes sense and I guess it explains the long times of the check.

@aconchillo
Copy link
Contributor

This PR #59709 will fix the issue once merged.

@chenrui333
Copy link
Member

@CamJN can you rebased your PR, I just merged the Agda fix PR.

@CamJN
Copy link
Contributor Author

CamJN commented Aug 23, 2020

Still getting this:

=> FAILED
40
Error: 1 problem in 1 formula detected
41
emacs:
42
  * Files were found with references to the Homebrew shims directory.
43
    The offending files are:
44
      libexec/emacs/27.1/x86_64-apple-darwin19.5.0/emacs.pdmp
45

I think this is due to emacs now using a new portable dumper instead of the abomination that was unexec.

@CamJN
Copy link
Contributor Author

CamJN commented Aug 24, 2020

Indeed, the portable dump file does refer to /usr/local/Homebrew/Library/Homebrew/shims/mac/super according to strings.

How does homebrew usually deal with things like this?

@CamJN
Copy link
Contributor Author

CamJN commented Aug 24, 2020

@CamJN
Copy link
Contributor Author

CamJN commented Aug 24, 2020

I rebuilt using the --with-dumping=unexec configure option, and ran find . -type f -exec fgrep -ai '/shims' {} \; -print which seems to not include a reference to the shims dir, except in the config.log file.

That said, that's not a viable strategy going forward as unexec is going away eventually.

@SMillerDev
Copy link
Member

What files does it reference?

@CamJN
Copy link
Contributor Author

CamJN commented Aug 24, 2020

@SMillerDev it just refers to the path to the shims super env dir not a file in there, in fact I believe (but don't know for sure yet) it is just capturing the PATH env var at the time it dumps the new executable.

@CamJN
Copy link
Contributor Author

CamJN commented Aug 24, 2020

@SMillerDev here's the strings output that makes me think it's just the PATH:

$ strings /usr/local/opt/emacs/libexec/emacs/27.1/x86_64-apple-darwin19.6.0/emacs.pdmp | rg -B 13 shims
/sbin
/usr/sbin
/bin
/usr/bin
/usr/local/opt/gnutls/bin
/usr/local/opt/libevent/bin
/usr/local/opt/openssl@1.1/bin
/usr/local/opt/p11-kit/bin
/usr/local/opt/nettle/bin
/usr/local/opt/libtasn1/bin
/usr/local/opt/libidn2/bin
/usr/local/opt/gettext/bin
/usr/local/opt/pkg-config/bin
/usr/local/Homebrew/Library/Homebrew/shims/mac/super

@aconchillo
Copy link
Contributor

aconchillo commented Sep 2, 2020

This is not directly related but in #60307 (comment) I believe I explained why Homebrew does this type of checks. Basically, when a bottle is built Homebrew doesn't want references to Homebrew prefix hardcoded in files, this is so at installation time a new prefix can be used.

Homebrew takes care of this in text files when building a bottle by replacing Homebrew path references with placeholders and replaces them back with the Homebrew prefix at installation time, but in this case it's a binary file so it can't do any of that.

I don't really know enough about Homebrew but this seems a tough one. Without knowing much, the only thing that comes to my mind is creating a link to the shims directory:

/usr/local/opt/homebrew-shims/ -> /usr/local/Homebrew/Library/Homebrew/shims/mac/super

or something like that and add the /usr/local/opt/... directory to PATH instead of /usr/local/Homebrew/.... This way the formula shims check will succeed and bottles will be able to install in different Homebrew prefixes as long as /usr/local/opt is fixed (i.e. the same in system where bottle is built and system where bottle is installed).

This of course involves changing Homebrew, so hopefully there's a much simpler solution. I use emacs in the terminal on my day to day work, so I'm very interested in this one 😄 .

@CamJN
Copy link
Contributor Author

CamJN commented Sep 2, 2020

I've been busy, but I have a theory that we can edit the PATH by loading some elisp before the executable is dumped. I'm going to try but I can't do it right now.

@aconchillo
Copy link
Contributor

I've been busy, but I have a theory that we can edit the PATH by loading some elisp before the executable is dumped. I'm going to try but I can't do it right now.

Great! Would that remove /usr/local/Homebrew/Library/Homebrew/shims/mac/super?

@CamJN
Copy link
Contributor Author

CamJN commented Sep 2, 2020

@aconchillo that is the plan, yes.

@aconchillo
Copy link
Contributor

I was curious and tried combinations of the following (this is just a test):

;; Remove Homebrew shims path from PATH since Homebrew doesn't
;; like having hardcoded paths to the Homebrew directories.
;; See https://github.com/Homebrew/homebrew-core/pull/59448#issue-465829006
(let ((env-path (getenv "PATH")))
  (message "Before PATH is %s" env-path)
  (message "Process environment is %s"
           (mapconcat 'identity initial-environment " "))
  (if env-path
      (setenv "PATH"
              (mapconcat 'identity
                         (delete "/usr/local/Homebrew/Library/Homebrew/shims/mac/super"
                                 (split-string env-path ":"))
                         ":")))
  (message "After PATH is %s" (getenv "PATH")))

in different parts of loadup.el without success. loadup.el is where emacs.pdmp is created, but (getenv "PATH") returns nil. I'll investigate more.

@CamJN
Copy link
Contributor Author

CamJN commented Sep 2, 2020

Rather than edit loadup.el, you want to add a site-load.el to the lisp/ dir. And yes, the getenv is where I last had issues.

@CamJN
Copy link
Contributor Author

CamJN commented Sep 3, 2020

Ok, I got rid of the reference to the homebrew shims dir, someone who is up to date on what homebrew wants stylistically should chime in on how they want this file added.

Formula/emacs.rb Outdated Show resolved Hide resolved
Formula/emacs.rb Show resolved Hide resolved
@aconchillo
Copy link
Contributor

@CamJN Oh... so it was exec-path. Nice!

Formula/emacs.rb Outdated Show resolved Hide resolved
@CamJN
Copy link
Contributor Author

CamJN commented Sep 3, 2020

Sweet, all checks passed. Do I need to squash or can you guys use the squash and merge button?

@SMillerDev
Copy link
Member

Not without breaking things. Please squash the commits.

@CamJN
Copy link
Contributor Author

CamJN commented Sep 3, 2020

Done

@CamJN
Copy link
Contributor Author

CamJN commented Sep 4, 2020

agda timed out on 10.15 but the formula change is the same that passed everything last time.

@CamJN
Copy link
Contributor Author

CamJN commented Sep 4, 2020

@SMillerDev is there a way to run just the failed test again? Or kick off all the tests? Not sure what to do about the agda test since that was supposed to be fixed and I rebased on master so I should have that commit included.

@CamJN
Copy link
Contributor Author

CamJN commented Sep 4, 2020

@chenrui333 any idea how to speed up the agda tests? They're timing out: https://github.com/Homebrew/homebrew-core/pull/59448/checks?check_run_id=1070317115#step:5:699

@aconchillo
Copy link
Contributor

@SMillerDev is there a way to run just the failed test again? Or kick off all the tests? Not sure what to do about the agda test since that was supposed to be fixed and I rebased on master so I should have that commit included.

Rebasing and pushing again should kick off all tests.

@CamJN
Copy link
Contributor Author

CamJN commented Sep 6, 2020

All checks passed again.

Copy link
Member

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

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

Thanks @CamJN ! Without contributions like yours it'd be impossible to keep homebrew going with the high standards that users have come to expect from the project. You can feel good knowing that you've made the world a tiny bit better for homebrew users around the world! 👍 🎉

@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

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.

None yet

7 participants