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

Python package #2361

Closed
wants to merge 2 commits into from
Closed

Python package #2361

wants to merge 2 commits into from

Conversation

gnpaone
Copy link

@gnpaone gnpaone commented Sep 12, 2024

I'm interested in #2311 python package as well since I use poetry for some functions, I have seen the existing PR and I understand the requested changes.
The Check if version tag checks for the x.y.z tag. I'm well-versed in CI/CD so I can help. Please lemme know any changes if required.

@casey
Copy link
Owner

casey commented Sep 14, 2024

On reflection, I'm very hesitant to add this. I have a bad track record of maintaining just packages myself, since it adds a lot of friction to releases, and they tend to get out of date easily. I also haven't had the greatest experience with the python package ecosystem, and I feel like this is likely to break, especially because I'm unlikely to actually test it after each release. The current rust release workflow is pretty simple, and doesn't tend to break, and I don't think I want to make it more complicated.

I think the best thing would be for someone to maintain the python package in another repo, and I can link to it in the readme, similar to other packages.

@casey
Copy link
Owner

casey commented Sep 14, 2024

For the package name, I would suggest rust-just, but justcli is fine too.

@gnpaone
Copy link
Author

gnpaone commented Sep 14, 2024

For the package name, I would suggest rust-just, but justcli is fine too.

Sure, I can maintain the package by creating a new repo, as soon as there's a release here I can setup pipelines that can test the package and release it. I'll update more details over here as soon as I setup the process if it's okay for you

@casey
Copy link
Owner

casey commented Sep 14, 2024

Sounds good!

@gnpaone
Copy link
Author

gnpaone commented Sep 20, 2024

pip install rust-just to install just via pip
npm package is also on the way and will publish tomorrow

@casey
Copy link
Owner

casey commented Sep 21, 2024

Nice! Added instructions to the readme #2383.

@gnpaone
Copy link
Author

gnpaone commented Sep 21, 2024

Here's npm package https://www.npmjs.com/package/rust-just :)

npm install rust-just will install the package

https://github.com/gnpaone/rust-just is the repo of both python and npm package xD

@casey
Copy link
Owner

casey commented Sep 21, 2024

Nice! Documented in #2384.

@brombal
Copy link
Contributor

brombal commented Nov 9, 2024

FWIW, I would like to comment/warn that npm i rust-just does not install the just binary globally for the user, it only installs it to the current node package that the user is working on (and if there isn't one, it creates a new node project in the current directory). The documentation for rust-just currently doesn't indicate this (or anything useful, really; in fact, the README documentation is just a copy of the entire just README).

This difference feels misleading, as it seems outside the spirit of the just README section for installing via a package manager, which lists ways to install it on a global level (asdf, brew and cargo are the ones I'm most familiar with; the others imply they do something similar).

Most worryingly, the https://github.com/gnpaone/rust-just appears to be a manual fork of casey/just, which, at best, will require consistent "syncing" of the source code by the maintainer (a potential source of frustration to users looking to upgrade their just version), or at worst, is an untrusted clone of the original source with potential for security vulnerabilities without the same active review as the original repo.

I hate to be "that guy" here, but the rust-just project seems to be either misguided in its intentions/implementation, or potentially even malicious or at least insecure - I'm not accusing, but just saying I personally could not trust it enough to use it without a thorough review (which I ain't got time for).

@casey please feel free to tell me to mind my own business, but I'd encourage you to review whatever is actually happening in the other repo and make sure it aligns with your project's goals.

#1238 (comment)

@gnpaone
Copy link
Author

gnpaone commented Nov 9, 2024

@brombal You can install rust-just globally using npm i rust-just -g and it's documented here at https://github.com/gnpaone/rust-just/tree/master/npm. I've added this readme (which also points out towards original readme instructions in casey/just) recently and from next release I can modify such that it shows this readme 🙂 instead of the original. The repo automatically syncs from source repo using a workflow and no manual syncing is done (similar to a fork but in the case of fork unless PR is created it doesn't count towards my contributions 😅). Also I'm not building my own binaries, I'm using the binaries from casey/just while packaging to npm registry as there's a workflow for that too. Also there's not really a necessity to sync the whole repo because I need only few files such as LICENSE, README, Cargo.toml etc for the purpose but I just wrote simple code to sync all and then checkout my repo and use whatever files necessary. You can take a look at my code and see my past commits for any presence of malicious modifications if you feel suspicious since it's open source. Sorry if I sound rude. I am a maintainer and it's just my way to use github workflows to the max with less to no manual intervention to keep my packages up-to-date.

I guess modifying the README here at https://github.com/casey/just/blob/master/README.md?plain=1#L170 to npm install -g rust-just would help the users @casey

@casey
Copy link
Owner

casey commented Nov 9, 2024

I think changing the readme to -g is probably a good idea. Feel free to open a PR!

It would also be a good idea, just for comprehensibility, to figure out how to avoid cloning the whole repo. Otherwise it's rather hard to figure out what's new and what's old.

In general, I just don't have time to review packages. If I had to review every package before linking it in the readme, I just wouldn't be able to link any packages because I wouldn't have any time to review them 😅 Packages are always kind of buy beware. I could see adding a small note above the packages table that says that they're packaged by third parties, and I don't review them. On the other hand, it doesn't seem like other projects actually do this.

@brombal
Copy link
Contributor

brombal commented Nov 9, 2024

@gnpaone Take this as just my two cents, but I don't believe npm is really intended as a system-level package manager for binaries that are not highly related to the node/npm ecosystem. Generally npm i -g is used for installation of common utilities that are meant specifically for node and JavaScript-related development (like tsc, eslint, etc). I'll admit that's definitely more of an opinion than a hard fact, though.

However, there are several caveats that I think could be misleading to users if not made extremely clear:

  • The rust-just binary always runs through a JavaScript intermediate script. Not always a big deal, but could definitely come with side-effects that the average user might not be able or wanting to deal with, especially if they are using rust-just with a different language.
  • There is a 3rd level of dependency on your repository and CI processes that appear to be maintained by a single contributor. I am absolutely guilty of the same thing, so I can't knock you for it; but if the added underlying complexity doesn't provide any significant benefit, I'd have to really consider whether it's useful to put this "single point of failure" in my dependency graph.
  • The user likely already had to install npm/node using one of the other available package managers listed, so do these additional complexities justify doing something that could be just as easy with a more appropriate package manager?

I definitely don't suspect any malicious intent here, but the average user is probably not going to dig deep enough to see that your repo is only an automated clone, or that most of the files are not being used. All they would see is a forked repo that is not maintained by the original just developers, and probably be rightfully scared off due to doubts about its authenticity.

If you can make these things obvious to users, and make it clear what the benefit is of installing via npm rather than a more typical system package manager, then at the very least you should make sure that the documentation at https://www.npmjs.com/package/rust-just is the rust-just documentation, and not the just README.

A more elegant solution might be to simply download and install the latest just binary (directly from the casey/just releases) in an npm postinstall script (a great example is https://github.com/eugeneware/ffmpeg-static - it doesn't have anything to do with the ffmpeg source code; it just uses a script to download the binaries and install them after running npm i ffmpeg-static)

And I definitely don't mean to be rude or mean-spirited about this at all either; I am just making comments as a engineering/security professional. I would likely have to deny this from being used in any professional capacity because of the points mentioned above.

@brombal
Copy link
Contributor

brombal commented Nov 9, 2024

In general, I just don't have time to review packages. If I had to review every package before linking it in the readme, I just wouldn't be able to link any packages because I wouldn't have any time to review them 😅 Packages are always kind of buy beware. I could see adding a small note above the packages table that says that they're packaged by third parties, and I don't review them. On the other hand, it doesn't seem like other projects actually do this.

@casey Good point, and I realize the things I've been mentioning are probably more of a discussion for gnpaone/rust-just and not for casey/just.

I guess my only concern here is that without a disclaimer in just's README, it does feel somewhat endorsed/official, but as a node "expert", I look at it and feel a little uneasy about it in its current state.

There is definitely a place for unreviewed 3rd party plugins/addons, and I don't really know what the standard is on providing disclaimers when it's clear they are 3rd party. But on the other hand, installers feel like a slightly different category, a bit too "essential" to be listed without a disclaimer if they're unreviewed.

As I said, I'm happy to just buzz off, I'm just looking out for my fellow node devs 😉

@gnpaone
Copy link
Author

gnpaone commented Nov 10, 2024

@brombal I totally get the concerns here—especially regarding npm as a system-level package manager for binaries that aren’t directly related to Node. My goal with rust-just was to offer a simple, alternative way to access just for users (including me) who are already in the Node ecosystem, particularly in situations where traditional package managers might not be available or practical. For instance, some common use cases for an npm-based installation approach such as Dockerized environments, deps management using package.json for tracking and installing all dependencies consistently across environments entirely via npm, npm-based auto-updates to packages using tools like Dependabot.

The current setup does, as you noted, use an intermediary JavaScript layer to run the binary via execa, which could introduce minor overhead. That said, I’ve tried to make the implementation as efficient as possible while sticking to standard Node practices. This is just an another option for users to use just.

I will work on clarifying the README with disclaimers and details about the repository's maintained by third party (which is me) and use the README related to npm only and not the copied README from this repo, changes will be reflected in next release. My intent is to make it clear that this is a third-party option tailored to npm users and not an official just distribution.

@brombal
Copy link
Contributor

brombal commented Nov 10, 2024

@gnpaone I would like to point out that your main use case of CI pipelines and making JS devs' experience easier has already been solved by just-install. It basically does what you are proposing but solves for (nearly) all of the problems I was discussing above:

Installing just globally via npm doesn't really make sense to me, but installing it as a local project dependency is useful for CI pipelines and JS dev experience. And it doesn't fork the just source code at all but instead just downloads the appropriate binary directly from the current just release. You also could install it globally with npm i -g, though I don't explicitly recommend it.

I am all for open source "competition" and don't want to simply come across as biased, but when two projects seemingly offer no clear distinction in functionality, yet one of them arguably has practical and security issues, referencing both of them in the casey/just README seems like a potential source of confusion, or at least the appearance of a lack of thoughtful review.

I'd ask that you consider whether the existing solution to this problem is sufficient for you, and if not, what does yours offer that the other does not. There could very well be a good use case for both projects to exist, by either serving different purposes or even providing competing approaches, I'm just not sure that I see what it is yet.

@gnpaone
Copy link
Author

gnpaone commented Nov 10, 2024

@brombal Please don't criticize or bad-mouth a package just because you think it's a competition for your package. There are differences and use cases for my package as well as yours. I can also criticize yours since your intention is criticizing—all that your package does is wrap a shell script in a node executable file and run it if linux and if windows it downloads, copies files, installs and deletes the installer. It's a script and not technically a package. What if I want to update just via npm such as using dependabot, run your script again? I'd better off create my own shell script or use the shell script you provided and run it rather than using a node wrapper since you are keen on criticizing "node wrapper" part. You are also using child_process to run the script, I'm using Execa which is an optimized version of the same. There are lots of differences so it's not a competition. You feel so bad about me using binaries from casey/just repo. You are downloading the binaries from github each and every time npm package is installed, I download once and package it in npm directly. If you are worried about so called security issues, running a third-party link (from github in this case) inside the npm package, modifying permissions inside a hidden shell script without user consent (in some cases the user account won't even have permissions to change access permissions) is a big security issue, is it not? As I said earlier npm package don't even concern with casey/just files, it's not for npm, and I said you can look at my previous commits.

I support open-source community and any feedback on improving my package is welcome but don't try to criticize or undermine any work and I am clear I don't see your package as competition cuz working and use case is different. I've been polite all along until you started bad-mouthing. Please forget about me criticizing your package part as I don't want to do that. And end of discussion for this matter!

@brombal
Copy link
Contributor

brombal commented Nov 10, 2024

Sorry @gnpaone I am absolutely not trying to criticize or bad mouth. I am only expressing technical opinions and constructive criticism. Intention and attitude are very hard to convey over textual comments, so I apologize if it seems rude or mean-spirited, because I don't mean it that way at all.

As I said, I am all in favor of friendly open source competition. If someone can contribute something different, useful, effective, and technologically sound, then nobody should be saying "not to" just out of of jealousy. That's different than voicing a concern or offering constructive criticism in a public forum over actual technological issues that might negatively affect unknowing users.

If you feel your project has value then I hope you continue working on it, and the community will of course decide what they want to use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants