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

remove lib/.nosearch #85

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@tarsius
Copy link
Contributor

tarsius commented Apr 8, 2016

This file was originally added to prevent a bundled cl-lib from
ending up on the load-path. But since then other libraries that
actually should be inside a directory on the load-path' were added andcl-lib` was removed.

I noticed that you require sly libraries using (require sly-X "lib/sly-X"). After applying this you should also be able to remove the "lib/sly-X".

remove lib/.nosearch
This file was originally added to prevent a bundled `cl-lib' from
ending up on the `load-path'.  But since then other libraries that
actually should be inside a directory on the `load-path' were added
and `cl-lib' was removed.
@joaotavora

This comment has been minimized.

Copy link
Owner

joaotavora commented Apr 9, 2016

I came very close to merging this this, but now I'm thinking I shouldn't.

If .nosearch were removed, everything would work OK. But I would have more elements in the load-path than I really need, but only in the package.el installation scenario, or in any scenario using normal-top-level-add-subdirs-to-load-path. Thus, the requires could not just be simplified. What if one just does?

(add-to-list 'load-path "dir/to/sly")
(require 'sly-autoloads)
M-x sly

It won't work. And I intend for a MELPAless install to remain possible.

So, to summarize: if .nosearch is to be removed, the require should be simplified, otherwise we have a redundant load-path. But for that, some code has to exist very early in sly.el to add lib to the load-path. I'm not sure what that code would be, but might be tricky considering compilation-time and load-time.

So I prefer to have this. The libs in lib are only meant to be loaded by SLY anyway.

For another perspective, some of those requires could possibly turn into loads... (this would possibly mess up other installation methods, so I'm not really considering it).

@tarsius

This comment has been minimized.

Copy link
Contributor

tarsius commented Apr 30, 2016

Sorry for the long delay!

But I would have more elements in the load-path than I really need

Two elements instead of one isn't really such a big deal I would say. Unless of course every package does it. But that isn't the case.

[...] It won't work.

That's because what you are doing here is highly unusual. Among the > 4500 packages on the Emacsmirror I think sly is the only one which does something like this.

What most users do is to put all libraries in a singe directory. Very often that's just the top-level directory, or when a subdirectory is used, then lisp/ is the most commonly used. (But in this case "lisp" may be too ambiguous and elisp/ should be used, or lib/ since that's what you already use.)

There's one exception to that convention: "bundled" libraries should be put into a separate directory, but let's discuss that in the other issue I opened.

@joaotavora

This comment has been minimized.

Copy link
Owner

joaotavora commented Apr 30, 2016

Your argument about best practices has value, but remind me again of the problem that arises with the current way i organize code.. Is it just the requires or is emacsmirror impacted in any practical way too?

Lib should indeed be elisp and slynk should be perhaps lisp. But then there is contrib as well... I'd rather not change it unless there is a good practical reason...

@tarsius

This comment has been minimized.

Copy link
Contributor

tarsius commented Apr 30, 2016

Is it just the requires or is emacsmirror impacted in any practical way too?

As part of the Emacsmirror I maintain a package database which can be queried using epkg.el. You can find a bit more information about the mirror and epkg on the Emacsmirror homepage.

The database contains among other things automatically extracted dependency information about all mirrored packages. Sometimes there are issues like feature conflicts or unsatisfied dependencies. My tool (primarily emir) automatically generate reports about such issues which I then try to address together with the involved upstream(s). This page lists the outstanding issues and also some of the "patches" in (1) that are use to fix, or when that is not possible at least "hide", issues.

The Emacsmirror is affected by various sly issues in that various manual corrections are required to make the data consistent, and every time you add a new sly-*.el library I will have to make another such adjustment. (This is because ignoring any libraries which are located inside a directory which also contains a .nosearch file has proven to be a very reliable heuristic to exclude libraries which should not be considered part of the package. sly is the only false-positive.)

And with regards to hyperspec the conflict is just hidden on the Emacsmirror. If some third package came along which requires that library, what package should it depend on sly or slime? So once two packages provide the same library and neither of these two packages is a dependency of the other, the only way to fix that is to extract that library into a separate package. (But that's the other issue I opened.)

But then there is contrib as well...

That doesn't have to be changed. It isn't uncommon to have e.g. lisp/ as well as contrib/ (or contrib/lisp/). What's more important is having all "essential libraries" in the same directory, which in this case would mean moving sly.el to lib. The name of that directory does not necessarily have to be changed. But lib/.nosearch should really be removed and the require forms be adjusted accordingly.

Just noticed you have also checked in sly-autoloads.el. That's a generated file which shouldn't be checked in.

@joaotavora

This comment has been minimized.

Copy link
Owner

joaotavora commented Apr 30, 2016

sly-autoloads.el is needed for a melpaless installation resulting from a git clone. What alternative do you have?

I wasn't aware of epkg.el but why can't you make an exception to tour .nosearch heuristic? Being an heuristic, it is bound to fail sometimes. I'm sure you can modify epkg.el with a few lines to improve that heiristic with a list of exceptions, like if there was an .epkg-dosearch file there. It's easier than try to change my development habits. Even if a bit bizarre in your opinion, they are legitimate.

The hyperspec issue is a different matter. I'll answer separately.

@joaotavora joaotavora closed this Apr 30, 2016

@tarsius

This comment has been minimized.

Copy link
Contributor

tarsius commented Apr 30, 2016

Even if a bit bizarre in your opinion, they are legitimate.

That's okay. But trying to have it changed is legitimate too. Didn't work, moving on.

@tarsius tarsius deleted the tarsius:dosearch branch Apr 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment