Closed ikappaki closed 3 years ago
Two workarounds for Emacs bug#48137 which results to 'package fns failing when passed a package file with DOS line endings.
I see the bug is not yet resolved upstream, but it seems your analysis of the problem is correct, so we don't have to wait for "official" solution to backport it. (Just a comment, nothing to do here.)
bin/boostra.el.part has been updated bin\eldev.bat.in has been updated
I think we have a problem here — not with your changes, but in general with Eldev. Currently, when you upgrade it, it never touches the launching script, you can only do that manually. Maybe we should include upgrading of the script into upgrade-self
first. I'll look into it this week.
with-eldev-normalized-package-file
This looks pretty complex and creates a temporary file. Can it be rewritten with advising (see the many usages of eldev-advised
in the existing code) package-install-file
instead?
Other changes look fine already, though I'd like to see comments from @juergenhoetzel first.
with-eldev-normalized-package-file
This looks pretty complex and creates a temporary file. Can it be rewritten with advising (see the many usages of
eldev-advised
in the existing code)package-install-file
instead?
I did try an advice at first, but it does not propagate to the --execute
script, as expected, when calling Emacs directly from test like test/package.el
tests:
(eldev--test-call-process "Emacs" eldev-emacs-executable
("--batch" "--no-site-file" "--no-site-lisp" "--execute"
`(progn
(require 'package)
(let ((package-user-dir ,test-emacs-dir)
;; Just use all we have, no need to
;; tailor for each test specifically.
(package-archives '(("archive-a" . ,(expand-file-name "package-archive-a/" (eldev--test-dir)))
("archive-b" . ,(expand-file-name "package-archive-b/" (eldev--test-dir))))))
(package-initialize t)
(package-refresh-contents)
(package-install-file ,package-file)
(package-activate ',(package-desc-name descriptor))
(prin1 (cadr (assq ',(package-desc-name descriptor) package-alist)))
,',child-emacs-form)))
;; ...
)
just for completion, below is a copy of the draft advice I used at an early stage. It does still have the same complexity as the macro. The complexity comes from the first parameter type to package-install-file
being a file path.
(define-advice package-install-file
(:around (old-fn file) package-install-file-hanlde-crlf)
(if (string-match "\\.el\\'" file)
(let* ((nondir (file-name-nondirectory file))
(temp-dir (make-temp-file "eldev" t))
(temp-file (expand-file-name nondir temp-dir)))
;; 'package only supports packages with LF line endings
(with-temp-buffer
(insert-file-contents-literally file)
(goto-char (point-min))
(while (search-forward "\r\n" nil t)
(replace-match "\n" nil t))
(write-region (point-min) (point-max) temp-file))
(funcall old-fn temp-file)
(delete-directory temp-dir t)
)
(funcall old-fn file)))
A much simpler alternative is to advise lm-header
(whose argument is a buffer), but this is a bit too indirect for my taste and as thus I am not much in favor of.
Please note that as far risk is concerned, the macro will only make a copy if it detects CRLFs in the input scream (DOS files), otherwise no copy is made.
Thanks!
I rather meant advising insert-file-contents-literally
to call insert-file-contents
instead (likely with a check against reentrance). I wrote it incorrectly.
I rather meant advising
insert-file-contents-literally
to callinsert-file-contents
instead (likely with a check against reentrance). I wrote it incorrectly.
Indeed, that is a much simpler approach, and it should work as the code currently stands in package-install-fine
, though we still have the issue with test/package.el:eldev--test-packaging
fn, with the advice not reaching out to Emacs --execute
, failing the tests. Would you like to consider a separate fix for the latter?
I guess the easiest would be to use with-eldev-normalized-package-file
(btw, rename it to e.g. eldev-with-package-install-file-fix
or ...-crlf-fix
, or something like that), only already macroexpanded, because the child process doesn't see the macro definition. As far as I see from documentation, macroexpand-all
is old enough. Alternatively, make it load eldev.el
for the definition.
Yet another idea: instead of the macro, just add a (wrapper) function called eldev-package-install-file
(or --
), with your fix/workaround. The original function is not called from anywhere else according to a quick grepping over Emacs source code. E.g. there is already eldev--package-dir-info
to backport the function to Emacs 24, but also to add several workarounds.
Fixed an issue with
webinstall/eldev.bat
that it fails when piped tocmd
as per the official instructions inREADME.adoc
. This is because the command line arguments%[1-9]
do not exist when piping tocmd
. (Hi @juergenhoetzel, has this ever worked for you or perhaps there is something different in my setup? I suspect it is the check for the additional argument that has broken it since).
Thanks. I can confirm this issue. And your commit fixed the pipe issue for me. But your changes in the last section don't work for me:
curl.exe -fsSL %URL% -o %ELDEV_BIN_DIR%\eldev.bat && (
echo Eldev startup script has been installed.
echo Don't forget to add `%ELDEV_BIN_DIR% to PATH environment variable:
echo.
echo set PATH=%ELDEV_BIN_DIR%;%%PATH%%
)
never actually downloads eldev.bat
when using it via | cmd
pipe.
Changing it to a if
expression works for me:
if curl.exe -fsSL %URL% -o %ELDEV_BIN_DIR%\eldev.bat (
echo Eldev startup script has been installed.
echo Don't forget to add `%ELDEV_BIN_DIR% to PATH environment variable:
echo.
echo set PATH=%ELDEV_BIN_DIR%;%%PATH%%
)
* The `.github/workflows/util.js` has been updated to reference the branch name rather than default to `master` when used in the workflow, thus enabling installation tests to reference the active branch.
@ikappaki would you mind to elaborate on the details of this change:
- const {repository, pull_request} = context.payload;
- const branch = pull_request ? pull_request.head.ref : repository.default_branch;
+ const {repository, pull_request, ref} = context.payload;
+ const branch = pull_request ? pull_request.head.ref : ref.replace(/refs\/.+?\//, "");
Is this for tests in forked users repos?
Yet another idea: instead of the macro, just add a (wrapper) function called
eldev-package-install-file
(or--
), with your fix/workaround. The original function is not called from anywhere else according to a quick grepping over Emacs source code. E.g. there is alreadyeldev--package-dir-info
to backport the function to Emacs 24, but also to add several workarounds.
I've replaced the macro with eldev-util.el:eldev--package-install-file
as per suggestion and forced emacs --execute
to load eldev-utils.el
so that it can reference it, thanks.
never actually downloads
eldev.bat
when using it via| cmd
pipe. Changing it to aif
expression works for me:if curl.exe -fsSL %URL% -o %ELDEV_BIN_DIR%\eldev.bat ( echo Eldev startup script has been installed. echo Don't forget to add `%ELDEV_BIN_DIR% to PATH environment variable: echo. echo set PATH=%ELDEV_BIN_DIR%;%%PATH%% )
Not sure why that didn't work for you, the if
statement now does not work for me :) . I've changed it slightly to simplify and make it a bit clearer by using ||
, which suppose to take effect when the command to its left fails. Could you please give it a try? If this doesn't work, then I've got something fundamentally wrong with my understanding of bat pipes.
I've also noticed that the commands were echoed when piped which is most confusing, so I've updated the documentation that it should be run with the `/Q' option. Thanks
* The `.github/workflows/util.js` has been updated to reference the branch name rather than default to `master` when used in the workflow, thus enabling installation tests to reference the active branch.
@ikappaki would you mind to elaborate on the details of this change:
- const {repository, pull_request} = context.payload; - const branch = pull_request ? pull_request.head.ref : repository.default_branch; + const {repository, pull_request, ref} = context.payload; + const branch = pull_request ? pull_request.head.ref : ref.replace(/refs\/.+?\//, "");
Is this for tests in forked users repos?
Yes, correct when I was running the workflow from my ikappaki test/webinstall
branch, it was trying to download files rom the master
branch rather the test/webinstall
branch. This was before I raised the PR while I was developing code in the branch. Thanks
I've also noticed that the commands were echoed when piped which is most confusing, so I've updated the documentation that it should be run with the `/Q' option. Thanks
Thanks :+1: This works for me!
Also :+1: for the /Q
switch. The echoed commands were quite confusing.
Yes, correct when I was running the workflow from my ikappaki
test/webinstall
branch, it was trying to download files rom themaster
branch rather thetest/webinstall
branch. This was before I raised the PR while I was developing code in the branch. Thanks
I see. LGTM :+1:
Sorry for the delay. As I understand, all issues mentioned by @juergenhoetzel and me had been fixed, so I merged this.
Should have squashed into one commit, but oh well, too late. Thanks for the fixes.
Please consider below fixes from observations made while trying to enable MS-Windows test support in CIDER.
Two workarounds for Emacs bug#48137 which results to
'package
fns failing when passed a package file with DOS line endings.bin/boostra.el.part
has been updated not loadeldev.el
"-literally" any more, thus enforcing the correct line endings in the buffer (CRLFs are treated as a single?\n
). This prevented the eldev project from bootstrapping after it was cloned from git with configcore.autocrlf=true
(the default setting for github and circleci workflows). The previous workaround forcing LF line endings in.github/workflows/test.yml
has thus been removed.eldev-util.el:with-eldev-normalized-package-file
macro is introduced to convert DOS line endings to UNIX of a package file in a new temporary file, if required, before passing it topackage-install-file
for evaluation. It is utilised ineldev-util.el:eldev-install-package-file
andtest/package.el:eldev--test-packaging
as necessary.run
blocks in.github/workflows/test.yml
so that failures are caught independently by the workflow (previously if main tests failed but integration tests succeeded would still show therun
block as green).The
.github/workflows/util.js
has been updated to reference the branch name rather than default tomaster
when used in the workflow, thus enabling installation tests to reference the active branch.bin\eldev.bat.in
has been updated to strip out double quotes (%~0
) fromELDEV_CMD
. It turns out that when invoked from PowerShell the path to program name is wrapped in double quotes. This resulted in eldev path errors when trying to determine the location of the eldev script.Introduced
webinstall/circle-eldev.ps1
to be used with circleci windows workflows, modelled as close as possible afterwebinstall/circle-eldev
. The choice of using a PowerShell vs. bat script here is because circleci's default windows shell is PowerShell.Fixed an issue with
webinstall/eldev.bat
that it fails when piped tocmd
as per the official instructions inREADME.adoc
. This is because the command line arguments%[1-9]
do not exist when piping tocmd
. (Hi @juergenhoetzel, has this ever worked for you or perhaps there is something different in my setup? I suspect it is the check for the additional argument that has broken it since).Thanks