-
-
Notifications
You must be signed in to change notification settings - Fork 84
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
cspell fails to install using nodeenv #53
Comments
@ekalinin do you have any idea what could have cause this? |
I can reproduce this without
|
I see, you are cloning the repo. To use
Please let me know if that works. |
@asottile I am a bit confused regarding where is the bug in this case. Shouldn't pre-commit be able to run build automatically or is cspell which should do this automatically when needed? My impression was that |
Is there a reason you are cloning the repository?
npm install -g cspell
Should install everything you need.
npm will have the latest release. Cloning master has the risk of breaking.
…On Thu, 5 Jul 2018 at 13:23, Sorin Sbarnea ***@***.***> wrote:
@asottile <https://github.com/asottile> I am a bit confused regarding
where is the bug in this case. Shouldn't pre-commit be able to run build
automatically or is cspell which should do this automatically when needed?
My impression was that npm install should work without extra magic
commands but I nodejs is not my area expertise.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#53 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADkR6TCrTciY6aWLGLAKLRHoV48WoxzIks5uDfclgaJpZM4VDBz_>
.
|
yeah the tool uses git revisions to enable easy forking / updating / etc. (It's also designed to work with a bunch of different languages so Every other npm package I can find works fine when running just Reading more into this, |
This patch seems to make diff --git a/package.json b/package.json
index e3f34fa..a9516a8 100644
--- a/package.json
+++ b/package.json
@@ -19,6 +19,7 @@
"lint": "tslint --force --format verbose \"src/**/*.ts\"",
"lint-travis": "tslint \"src/**/*.ts\"",
"build": "npm run compile",
+ "prepare": "npm run compile",
"clean-build": "npm run clean && npm run build && npm run build-dictionaries",
"build-dictionaries": "npm run build_dictionaries-word-lists",
"build_dictionaries-word-lists": "node node_modules/cspell-tools/dist/app.js compile \"./dictionaries/!(words)*.txt\" -o ./dist/dictionaries/", |
The package was using |
@Jason3S I am afraid that even after this change cspell does not install successfully inside nodeenv if it was not build a-priori. The original reproduction steps from the bug description are still valid and produce the same failure. It is critical to assure that cspell can be install in an isolated environment in order to be able to use it later. |
Avoids failure to install when compilation was not run yet. Fixes: streetsidesoftware#53 Signed-off-by: Sorin Sbarnea <ssbarnea@redhat.com>
Avoids failure to install when compilation was not run yet. Fixes: streetsidesoftware#53 Signed-off-by: Sorin Sbarnea <ssbarnea@redhat.com>
@ssbarnea why cannot you run
|
@Jason3S I am almost sure that adding "npm install" in-between would be a workaround but I am afraid this is not the way to do it. Is like like installing it twice and also performing actions outside the virtual environment, which can have undesired side-effects. A package should be installable directly, without extra operations. I asked on https://stackoverflow.com/questions/51283422/how-to-correctly-compile-a-nodejs-package-before-installing-it?noredirect=1#comment89544439_51283422 but it may take some time to get a solution. I checked 3-4 other nodejs packages and apparently none of them needed this bit strange compilation phase. I am not sure if @asottile can and wants to add some extra steps into pre-commit node support in order to accommodate for this non standard installation method. |
have to agree with this -- there has to be a way to make this work, going to look at some other packages and figure out how this is typically done |
You are using an anti-pattern by directly using the repository. You should only be using the published package. The published package has everything you need. The repository is not the package, the package is what is on NPM: https://www.npmjs.com/package/cspell There seems to be something fundamentally wrong with nodeenv if it cannot repackage a published npm package into a virtual environment. |
Why doesn't this work for you?:
|
pre-commit is a git hooks framework which supports many different programming languages. As a decent lowest common denominator that all the current languages support, it uses Installing from source is a pretty common operation even outside of this for site-specific customizations. Every other npm project I can currently find supports installation from source (even I'm fairly certain there is an easy fix here, just needs some more investigation. An aside, I encourage you to be a little more polite, we're just trying to make the ecosystem a better place and throwing out "using an anti-pattern" and "something fundamentally wrong" isn't productive, polite discourse :) (I understand there's no COC for this repository -- but we're just trying to help!). It's possible too that I've misconstrued your words above as well (and if that's the case I apologize!), text is a really difficult medium! |
As long we all work together we should be fine. I love both projects so I hope we will find a way to make it work. I urge both of you not to get personal on this and focus on finding ways to address it. There is no pressure to make it work today or tomorrow When I encountered pre-commit first time I found the git installation bit peculiar as the "norm" was to install a tool was to use package managers (like pip or npm). Now that I know more about it I do understand why git was picked in the first place: first is universal and not platform dependent. Also as opposed to package managers it gives some extra security by relying on hashes. We all know that there were previous hacks on centralised package repositories which ended injecting malware in packages. With git+hashes this is almost impossible to happen because once a consumer points to a hash, this will remain there and only the same code would be allowed to install. Shortly what looked weird to me couple weeks ago started to appear as beautiful design once I started to understand it better. Apparently both pip and npm allow installation of packages directly from git. It is true that cspell is the first package I encounter where this doesn't work, but I am also sure is not the only one. Now the challenge is to find others that had the same problem and resolved it. Maybe @ekalinin knows something about this. |
Based on recent feedback I got from few sourced I am inclined to believe that the bin script should be a wrapper instead of the result of a build process. This should allow it to install and also allow to run a post-install step that builds the other files needed, after installation, in order to avoid the chicken-egg problem. |
Why won't this work?
The issue is that this git repository is a repository for the source code. The source code is in TypeScript. When this repository is published, it is compiled, tested and packaged. This is a standard process. Nothing new. What is different between this repo and many other node repos is that the code was not written in JavaScript. If it had been written in JavaScript, then it would most likely (but not guaranteed) to have worked in the way you describe. Repos written in CoffeeScript or any of the other variants will have this issue if they do not check-in compiled code. It is not a standard practice to check in compiled code. It creates large diffs / pull requests that are not easy to review. |
Of course, commiting compiled code is a shot in the foot. @Jason3S The package contains problems with installation. I would expect this to work:
This won't work because package.json contains
The log is:
I suppose there should be static entry point that exists regardless of
|
@bisubus that makes sense because NPM has special behavior when installing to I would welcome a pull request that adds a I'm at work, so it is not easy for me to make the change myself. |
Once the
|
@Jason3S Sure. I don't use cspell myself by I'd expect this to be enough. |
This might have broken when the repo moved to a monorepo. Please use this instead: |
This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
While trying to enable cspell to be used a pre-commit hook, I faces a new issue, failure to install when doing:
The last command fails with:
The text was updated successfully, but these errors were encountered: