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

tools: Fix emacs compiler warnings (#4451) #4542

Merged
merged 2 commits into from
Feb 26, 2021
Merged

tools: Fix emacs compiler warnings (#4451) #4542

merged 2 commits into from
Feb 26, 2021

Conversation

leoliu
Copy link
Contributor

@leoliu leoliu commented Feb 22, 2021

Do not depend on cl. Assume presence of before-save-hook (Emacs 22)
and remove erlang-check-module-name-init. Remove (run-hooks
'erlang-mode-hook) from erlang-mode definition because
define-derived-mode does it already. Replace erlang-caddr with caddr
from Emacs 26.

erlang.el now compiles cleanly in Emacs 24.5, 25, 26, 27.

Do not depend on cl. Assume presence of before-save-hook (Emacs 22)
and remove erlang-check-module-name-init. Remove (run-hooks
'erlang-mode-hook) from erlang-mode definition because
define-derived-mode does it already. Replace erlang-caddr with caddr
from Emacs 26.

erlang.el now compiles cleanly in Emacs 24.5, 25, 26, 27.
@CLAassistant
Copy link

CLAassistant commented Feb 22, 2021

CLA assistant check
All committers have signed the CLA.

@rickard-green rickard-green added the team:PS Assigned to OTP team PS label Feb 22, 2021
@dgud dgud added the testing currently being tested, tag is used by OTP internal CI label Feb 24, 2021
@dgud
Copy link
Contributor

dgud commented Feb 25, 2021

The test case emacs_SUITE:compile_and_load still fails on emacs-27.1, not really your problem but you know much more about
emacs lisp than me, would appreciate if you have the time to take a look.

The same testcase fails on emacs-25.2 which it didn't before:

    space_Ω_/lib/erlang/lib/tools-3.5/emacs/erlang.el:6302:1:Warning: the
    function ‘caddr’ is not known to be defined.
../../../otp/elib with space_Ω_/lib/erlang/lib/tools-3.5/emacs/erlang.el:6302:1:Error: Symbol’s value as variable is void: %s
>>Error occurred processing /ldisk/daily_build/24_master-opu_o_bindir.2021-02-24_21/otp/elib with space_Ω_/lib/erlang/lib/tools-3.5/emacs/erlang.el: Symbol's value as variable is void ((%s))

@leoliu
Copy link
Contributor Author

leoliu commented Feb 25, 2021

I looked at emacs_SUITE.erl and it seems this line might be problematic. Not sure if that's the culprit though.

diff --git a/lib/tools/test/emacs_SUITE.erl b/lib/tools/test/emacs_SUITE.erl
index e5587b35..f74a159c 100644
--- a/lib/tools/test/emacs_SUITE.erl
+++ b/lib/tools/test/emacs_SUITE.erl
@@ -107,7 +107,7 @@ compile_and_load(_Config) ->
                 %% Workaround byte-compile-error-on-warn which seem broken in
                 %% Emacs 25.
                 "\"(advice-add #'display-warning :after "
-                    "(lambda (_ f _ _) (error \"%s\" f)))\"";
+                    "(lambda (_ f &optional _ _) (error \\\"%s\\\" f)))\"";
             _ ->
                 "\"(setq byte-compile-error-on-warn t)\""
         end,

I also found erldoc.el compiles with a warning under emacs 27. Is this what's causing the failure? Though the warning can be suppressed with the following simple change it might be a regression on emacs' side. I will check with emacs upstream to see what's happening to eval-and-compile:

diff --git a/lib/tools/emacs/erldoc.el b/lib/tools/emacs/erldoc.el
index a8ac81ec..5d185e1f 100644
--- a/lib/tools/emacs/erldoc.el
+++ b/lib/tools/emacs/erldoc.el
@@ -62,6 +62,7 @@
 
 (eval-when-compile (require 'url-parse))
 (require 'cl-lib)
+(require 'json)
 (require 'erlang)
 
 (eval-and-compile                       ;for emacs < 24.3
@@ -268,7 +269,6 @@ (defun erldoc-parse-all (man-index output &optional json)
     (with-temp-buffer
       (if (not json)
           (pp table (current-buffer))
-        (eval-and-compile (require 'json))
         (let ((json-encoding-pretty-print t))
           (insert (json-encode table))))
       (unless (file-directory-p (file-name-directory output))

@dgud
Copy link
Contributor

dgud commented Feb 25, 2021

You can run with make test ARGS="-suite emacs_SUITE" from $ERL_TOP/lib/tools

Yes warnings are errors if emacs > 25 and I understand the testcase correct.

And it fails on erldoc:

emacs --batch --quick --directory "/ldisk/daily_build/24_master-opu_o.2021-02-24_21/otp/lib/erlang/lib/tools-3.5/emacs" --eval "(require 'erlang-start)" --eval "(setq byte-compile-error-on-warn t)"  -f batch-byte-compile "/ldisk/daily_build/24_master-opu_o.2021-02-24_21/otp/lib/erlang/lib/tools-3.5/emacs/erldoc.el"

 => 1 Loading erlang-skels...
In toplevel form:
../../../otp/lib/erlang/lib/tools-3.5/emacs/erldoc.el:534:1:Error: the following functions might not be defined at runtime: json-encode, cl-reduce, cl-mapcan

The other modules have warnings as well, but doesn't seem to fail the testcase, I have not had time to dig deeper yet.

@dgud dgud removed the testing currently being tested, tag is used by OTP internal CI label Feb 25, 2021
@leoliu
Copy link
Contributor Author

leoliu commented Feb 25, 2021

Thanks for the tip make test ARGS="-suite emacs_SUITE". After some trial and error I was able to fix the failed tests and some warnings and errors that I found in the logs.

Somehow (eval-when-compile (require 'url-parse)) has to be moved to after (require 'cl-lib) in erldoc.el to address the following functions might not be defined at runtime: cl-reduce, cl-mapcan.

@dgud dgud changed the base branch from master to maint February 25, 2021 15:18
@dgud dgud added the testing currently being tested, tag is used by OTP internal CI label Feb 25, 2021
@dgud dgud merged commit 0bdf15c into erlang:maint Feb 26, 2021
@dgud
Copy link
Contributor

dgud commented Feb 26, 2021

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:PS Assigned to OTP team PS testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants