emacs-eask / cli

CLI for building, running, testing, and managing your Emacs Lisp dependencies
https://emacs-eask.github.io/
GNU General Public License v3.0
138 stars 21 forks source link

It seems Eask uses same environment for byte-compiling all the files #162

Closed Fuco1 closed 1 year ago

Fuco1 commented 1 year ago

I'm getting some strange results, for example files not requiring some libraries still byte compile with no issues through eask compile even though they should produce warnings about functions not being defined.

When files are compiled one by one they produce warnings.

You can download the project here https://gist.github.com/Fuco1/bf249d8106a3fd3c20ed98e9c02b79df

image

Fuco1 commented 1 year ago

See also this comment about makem.sh https://github.com/emacs-elsa/Elsa/issues/216#issuecomment-1468517165

jcs090218 commented 1 year ago

Sounds good to me! I will add it later today.

eask compile --clean

Would this be sufficient?

Fuco1 commented 1 year ago

I think what is needed is to compile each file always in a separate Emacs process as explained in the linked comment.

I've noticed some other issues when some macros did not expand correctly but when I compiled files individually they did (with-ansi from ansi package).

jcs090218 commented 1 year ago

I think what is needed is to compile each file always in a separate Emacs process as explained in the linked comment.

I'm not sure if it's easy to do within Eask, but I can try. 👍

I've noticed some other issues when some macros did not expand correctly but when I compiled files individually they did (with-ansi from ansi package).

Maybe Eask uses its' own ansi.el? https://github.com/emacs-eask/cli/blob/master/lisp/extern/ansi.el. I couldn't get ansi-inhibit-ansi to work so I have tweaked a little. 🤔

Fuco1 commented 1 year ago

I couldn't get ansi-inhibit-ansi to work so I have tweaked a little.

Yes it was bugged. I actually patched it and it's in the ansi.el master now :) (I use it in elsa and it works).

But this "proves" that eask can actually compile the files in a wrong environment using its own dependencies.

jcs090218 commented 1 year ago

Yeah, the goal is to keep those dependencies up to date. I tried to update it few days ago, but I got into some error (the latest ansi.el) so reverted back. I'll try it again later today. ;)

jcs090218 commented 1 year ago

Yeah, the goal is to keep those dependencies up to date. I tried to update it few days ago, but I got into some error (the latest ansi.el) so reverted back. I'll try it again later today. ;)

Updated in #163.

But this "proves" that eask can actually compile the files in a wrong environment using its own dependencies.

Yeah, ansi.el (fully) is the only library that Eask depends on, but we could easily resolve this by dragging (from internet) the file when the program start. Other than this, Eask wouldn't interfere the environment. The proof is, the results are the same between eask pacakge && eask install and eask compile.

🔍 eask pacakge && eask install

(Total of 1 dependency installed, 0 skipped)
Searching for artifact to install...
  Found artifact in d:/_workspace/test-1/dist/eask-test-1.0.0.tar
Parsing tar file...
Parsing tar file...done
Parsing tar file...
Parsing tar file...done
Parsing tar file...
Parsing tar file...done
Extracting... \
Extracting...done
  INFO     Scraping files for loaddefs...
  INFO     Scraping files for loaddefs...done
+ Checking d:/_workspace/test-1/.eask/29.0.60/elpa/eask-test-1.0.0...
Compiling d:/_workspace/test-1/.eask/29.0.60/elpa/eask-test-1.0.0/eask-test-autoloads.el...
Compiling d:/_workspace/test-1/.eask/29.0.60/elpa/eask-test-1.0.0/eask-test-pkg.el...
+ Compiling d:/_workspace/test-1/.eask/29.0.60/elpa/eask-test-1.0.0/one.el...
+ Compiling d:/_workspace/test-1/.eask/29.0.60/elpa/eask-test-1.0.0/two.el...
Done (Total of 2 files compiled, 2 skipped)

🔍 eask compile

  Loading Eask file in d:/_workspace/test-1/Eask... done!

+ Compiling d:/_workspace/test-1/eask-test.el... done
+ Compiling d:/_workspace/test-1/one.el... done
+ Compiling d:/_workspace/test-1/two.el... done

(Total of 3 files compiled, 0 skipped)

If we try comment out the (require 'dash) in one.el:

🔍 eask pacakge && eask install

Searching for artifact to install...
  Found artifact in d:/_workspace/test-1/dist/eask-test-1.0.0.tar
Parsing tar file...
Parsing tar file...done
Parsing tar file...
Parsing tar file...done
Parsing tar file...
Parsing tar file...done
Extracting... \
Extracting...done
  INFO     Scraping files for loaddefs...
  INFO     Scraping files for loaddefs...done
Checking d:/_workspace/test-1/.eask/29.0.60/elpa/eask-test-1.0.0...
Compiling d:/_workspace/test-1/.eask/29.0.60/elpa/eask-test-1.0.0/eask-test-autoloads.el...
Compiling d:/_workspace/test-1/.eask/29.0.60/elpa/eask-test-1.0.0/eask-test-pkg.el...
Compiling d:/_workspace/test-1/.eask/29.0.60/elpa/eask-test-1.0.0/one.el...

In end of data:
+ one.el:3:2: Warning: the function `-filter' is not known to be defined.
Compiling d:/_workspace/test-1/.eask/29.0.60/elpa/eask-test-1.0.0/two.el...

In end of data:
+ two.el:3:2: Warning: the function `-filter' is not known to be defined.
Done (Total of 2 files compiled, 2 skipped)

(Installed in d:/_workspace/test-1/)

🔍 eask compile

  Loading Eask file in d:/_workspace/test-1/Eask... done!

Compiling d:/_workspace/test-1/eask-test.el... done
Compiling d:/_workspace/test-1/one.el... done

In end of data:
+ one.el:3:2: Warning: the function `-filter' is not known to be defined.

Compiling d:/_workspace/test-1/two.el... done

In end of data:
+ two.el:3:2: Warning: the function `-filter' is not known to be defined.

(Total of 3 files compiled, 0 skipped)

The results matched and were accurate, and this is design by default.

Overly using the require macro does not mean it's good; it could lead to some errors, like circular dependency problem, etc. Of course, the example project https://gist.github.com/Fuco1/bf249d8106a3fd3c20ed98e9c02b79df you gave is considered too "small" and "simple", so it's hard to encounter such a problem. But for projects like helm or lsp-mode, it's more likely going to cause that kind of error.

Anyways, I've implemented this feature since I think it's nice to have for smaller projects! :D See below,


I think what is needed is to compile each file always in a separate Emacs process as explained in the linked comment.

Opened a PR in #166. You can now use the --clean option to accomplish such a behavior.

Notice this wouldn't be the default since I think the default should match the result from what package-build gave you.

Fuco1 commented 1 year ago

Overly using the require macro does not mean it's good; it could lead to some errors, like circular dependency problem, etc.

Circular dependency is a problem with the code layout and needs to be resolved anyway. You can't have circular dependencies logically.

Notice this wouldn't be the default since I think the default should match the result from what package-build gave you.

The order is not really determined though. If you switch the require from one.el to two.el you get an error. The order is either alphabetical or just random in some other way. Both of these cases are bad, because one does not require two and two does not require one so nobody makes sure dash in fact is loaded when the other is loaded. So in each of them a warning must be produced.

image

jcs090218 commented 1 year ago

Yeah, you are right. 👍 Just pointing out what could happen in the real project. ;)

jcs090218 commented 1 year ago

You can achieve this with the --clean flag; therefore, I am closing this issue! 👍 Thanks for the feature request! Eask is getting better and better! :D