-
Notifications
You must be signed in to change notification settings - Fork 40
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
RFC: FPM packaging #484
base: main
Are you sure you want to change the base?
RFC: FPM packaging #484
Conversation
I see some distribution inference rules that seemed to pop up several times. The script seems to be useful in several places, but I have not deduplicated the code yet. That would involve changes that are out of scope.
Deployment would be easier if `pa.sh` helped users with `PASH_TOP` and a default `~/.pash_init`. I saw scripts to modify the user's rcfiles (which is not ideal, imo). With process substitution, the process for setting `PASH_TOP` is a little nicer. $ cd "$(find ~ -type f pa.sh | xargs -n 1 dirname | head -n 1)" $ ./pa.sh # Add this to your shell configuration. export PASH_TOP='/home/sage/prj/pash' export PATH="$PASH_TOP:$PATH" # If your shell supports process substitution (bash, zsh, etc.) # run `. <(./pa.sh)` to use PaSh in this shell. The key benefit for deployment is that you can simply instruct a user to actually start using PaSh and follow its instructions, as opposed to adding installation steps. Since this is Bash, I use `return` to prevent accidental execution of further instructions when `pa.sh` is sourced.
Some code only calls `make` with no target specified. No `autogen.sh`, `configure`, etc. Maybe that's fine for the runtime, but it's not enough for the `libdash` derivative. I wonder if there's a different script that I haven't seen yet that does this better.
Question: With C in mind, what target architectures are we talking about? I'm assuming |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far, left one comment. Tagging Michael (@mgree ) who is working on simplifying the installation of libdash, since you mentioned that you are struggling with this.
I think that Michael is planning to have a working clean installer for libdash by the end of March. Michael, am I saying this right?
pa.sh
Outdated
|
||
|
||
rcfile=~/.pash_init | ||
if [ ! -f "$rcfile" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to do those everytime? Or maybe just with a pash-init.sh script? I am afraid that they will add to the overhead of the shell script. Also, I don't think we should print to stdout (see 3 lines below).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT: This code is here because it's not suited for system-level deployment. Is it better to use a command line flag here, or ship a configuration helper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Configuration helper is likely the way to go (for now we can have it in setup-pash.sh
?), as we're trying to minimize startup overheads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Configuration helper is likely the way to go (for now we can have it in
setup-pash.sh
?), as we're trying to minimize startup overheads.
setup-pash.sh
is not suitable for post-install steps because it mixes user-level and system-level commands, and starts from the user's perspective. If I did the same:
$ sudo apt install pash # ~ is not correct.
$ pa.sh # Not configured for the user
Putting the code in pa.sh
was meant to avoid adding an additional setup command, but shell configuration must happen on the user's command. I was going for this:
$ sudo apt install pash # Installs
$ pa.sh # Asks for configuration initially, then helps
Yes---my plan is to have something like We would want to change |
Gotcha. I'm doing something different. Here, PaSh is a system level package, so |
Last I heard anything about it, PaSh was vendoring dependencies in its own directory rather than installing on the system properly. My plan is to make libdash a conventional |
What you are making is fantastic, because we can simply list it in |
And yes, you are correct, the Python packages are not properly set up. It all comes from difficulty organizing PaSh for use as a Python package itself. Moving to system packages solves a lot of problems you're having, and brings the focus towards distribution-specifics. |
Got libdash compilation working on my machine in 3191b89 |
Nice! That means porting to my stuff should be easy, when it's ready. My cleanup work is back on the original repo, at mgree/libdash on branch python-bindings. I don't have the |
Merge conflicts fixed, reopening for discussion and testing in light of new changes. Any comment I did not address here was addressed in an email, so please link me to any questions you'd like me to address here specifically. I'm removing |
Repeating the commands from the docs used to build and test the packages.
Good news: No major change in behavior. Fedora and Arch launch PaSh successfully but without dictionary data (due to lack of I still see the behavior @dkarnikis explains in #484 (comment) . I think this is out of scope for merging. |
I manually approved the DCO sign-off, since A) I didn't know of the requirement until reading the details now, and B) I could certify I wrote the commits here with my manual approval to avoid a long rebase. If you'd rather I amend all of the commits, please let me know. |
I've been told that fixing `pa.sh`'s fallback value for `PASH_TOP` would cause performance penalties. This patch updates `PASH_TOP` in `pa.sh` as a post-installation step for packages Verified with existing Docker images, but do not assume 100% compatibility with all package managers. Unsure if there are special rules out there that prevent an installer writing to a file extracted from a package.
Add GitHub Actions paragraph content
Package PaSh for system-level package managers using FPM
Update: 2022 March 10
I learned that macOS is not supported a bit late, so I'm stopping macOS work. I need acceptance criteria for this PR, so that I know what to do before this is ready to merge.
Update: 2022 March 7
Fedora working (without dictionary data) as of ee36b6b
Starting macOS. This will be the last supported platform on my end.
Update: 2022 March 4
Debian, Ubuntu, and Pop_OS! are working. Fedora has a problem with the the
mv
hack for Pythonsite-packages
anddist-packages
. Arch lacks a dictionary for the intro example, butpa.sh
runs.This one is fun to review with
tmux
.The system obsoletes many deployment scripts, but that doesn't really matter until packages are hosted somewhere.
Please review and let me know what you'd need to merge this as a feature.