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

feat: Adding self-extracting file as an optional feature #31

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

prsabahrami
Copy link

Motivation

Closes #4

Changes

Similar to conda-constructor, I have added a header.sh file. After the environment has been packed and archived, that archive is appended onto header.sh which later extracts it when it runs.

Packing an environment

pixi-pack pack --create-executable

Unpacking an environment

either directly run the .sh file created

sh environment.sh

You can use below flags

Unpacks an environment packed with pixi-pack

-f           no error if environment already exists
-h           print this help message and exit
-p ENV       environment prefix, defaults to $PREFIX
-i INSTALLER create the environment using the specified installer defaulting to $INSTALLER
-a           create an activation script to activate the environment

or you can directly use

pixi-pack unpack environment.sh

I will try to add better tests and error handling in the upcoming days. Meanwhile, let me know if you have any recommendations.

@pavelzw pavelzw changed the title Adding self-extracting file as an optional feature feat: Adding self-extracting file as an optional feature Jul 25, 2024
@0xbe7a
Copy link
Contributor

0xbe7a commented Jul 25, 2024

One of the biggest use-cases for us for pixi-pack is the ability to build up archives on Linux for foreign targets, like Windows and to extract it there. Having a self-extracting archive there would be probably the most useful scenario. Your prepended shell script will only work for Unix systems, right?

Also after taking a (brief) look it appears that the extraction script is expecting a existing conda or micromamba executable on PATH. I think in these scenarios it's already an attractive option to manually extract the archive and create the environment using the included lockfiles. I think it would be very useful, if we could prepend a standalone extraction tool, that does not require any system tools to be available on the system.

@prsabahrami
Copy link
Author

@0xbe7a
Yes, it is correct.
I was thinking I could add a bat self-extracting script to be used on Windows in a separate PR. Then through the passed --platform we can decide to generate sh script or bat script (or both).
Please let me know if this works for the mentioned scenario.

Also, in regards to the extraction tool used in the script, fair enough.
I was thinking to implement the logic used in unpack.rs onto the script(s) so we could get rid of conda and micromambda. Let me know what you think. I'm not sure how efficient the script will turn out.

@0xbe7a
Copy link
Contributor

0xbe7a commented Jul 26, 2024

The part that involves extracting and linking the packages is highly non-trivial and probably not really feasible with a bash script. Instead, we should try using rattler as we did in pixi-pack itself. rattler supports all the platforms we target. The approach I am considering involves including an executable object with the required unpack functionality and using this when unpacking the archive to make it truly standalone.

Additionally, upon reviewing your pull request again, please note that micromamba shell activate already exists and can create the entire activation script for micromamba. For unpacking the archive using micromamba, you can use micromamba to create the environment and then generate the activation script to get the final prefix. This way, a single shell script can be used to activate into this environment, similar to what pixi-pack unpack would have produced.

@prsabahrami
Copy link
Author

Sounds good.
I will include an executable using rattler onto the script. Which the script will use to properly extract the archive.

Also, in regards to the micromamba shell activate, I will try to use rattler_shell inside the executable to create the activation script so we can remove the necessitiy of conda or micromambda.

@prsabahrami
Copy link
Author

prsabahrami commented Aug 5, 2024

Apologies for the delay. Got busy with school work.
I have added an extractor subproject which then is built and its binary is added to script.
The original commands still work but in case the -i conda is passed to the script, one can not create an activation script since I'm using micromamba or rattler to create the script. If needed, I can modify header.sh to extract extractor binary at all times but I thought if someone has conda installed on their devices, there is no need to take extra time and extract the binary.
Let me know what you think and then I can improve the verbosity and testing.
Note: The logic used in extractor is just a simpler version of what is in unpack.rs.

@pavelzw
Copy link
Member

pavelzw commented Aug 7, 2024

there is no need to take extra time and extract the binary

I think it would be better for reproducibility if we always unpack the same way, i.e. always use extractor

@pavelzw
Copy link
Member

pavelzw commented Aug 7, 2024

Could you fix the merge conflicts as well?

@prsabahrami
Copy link
Author

Could you fix the merge conflicts as well?

Done.

@prsabahrami
Copy link
Author

Added the tests from unpack.rs to extractor.
One thing I don't like is that the dependencies of extractor are being defined separately and hence built and compiled twice during the build process. Usually, using [workspace] solves this in Cargo.toml but I couldn't make it work using [workspace]. Any suggestions? This is making the build somewhat slow.

@prsabahrami
Copy link
Author

Fixed the merge conflict.
Any idea on the previous comment?

@prsabahrami
Copy link
Author

As I mentioned above, the issue here is with the lockfile conflicts and incorrect dependencies between extractor and pixi-pack. I am not sure how to correctly take care of this but for now, I have updated the dependencies and made extractor compliant with rattler's latest version. But once again, if the lockfile of the main project changes, I am not sure how it will affect extractor.

extractor/Cargo.toml Outdated Show resolved Hide resolved
src/header.sh Outdated Show resolved Hide resolved
src/header.sh Show resolved Hide resolved
@pavelzw pavelzw added the enhancement New feature or request label Sep 13, 2024
@prsabahrami prsabahrami marked this pull request as draft September 21, 2024 22:07
@prsabahrami prsabahrami marked this pull request as ready for review September 22, 2024 15:16
@prsabahrami
Copy link
Author

@pavelzw I have added a header.ps1 which adds support for windows now.
Let me know what do you think!

@prsabahrami
Copy link
Author

@pavelzw Any update on this?

Copy link
Member

@pavelzw pavelzw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This already looks very good @prsabahrami! Keep up the good work :)

Some minor comments

@@ -17,6 +17,7 @@ repos:
types: [text]
- id: end-of-file-fixer
name: end-of-file-fixer
exclude: src/header.(sh|ps1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we do this?

src/header.sh Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could add shellcheck as an additional pre-commit hook 🤔

-q, --quiet Decrease logging verbosity
"
# Parse command-line options
while getopts ":hfvo:s:q" opt; do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getopts is not available everywhere. for example: docker run --rm -it ubuntu which getopts has exit code 1

wondering whether there is a nice alternative for this or whether we would need to implement this ourselves

;;
esac
done
shift $((OPTIND -1))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this do? (ideally also write a comment 😅)


Unpacks an environment packed using pixi-pack

-f, --force No error if environment already exists
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my comments in header.sh


# Step 1: Extract the archive and pixi-pack executable, and decode them
$scriptContent = Get-Content -Raw -Path $MyInvocation.MyCommand.Path
$lines = $scriptContent -split "`r?`n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this \r\n? 😅

}

if (-not $headerLine -or -not $archiveLine) {
Write-Error "Markers __END_HEADER__ or __END_ARCHIVE__ not found."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think we have a similar test in header.sh. do you think we can add it there as well?

$archivePath = "$TEMPDIR\archive.tar"
[System.IO.File]::WriteAllBytes($archivePath, $decodedArchive)
} catch {
Write-Error "Failed to decode Base64 archive content: $_"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does $_ do?

# Finally, add the path to the archive
$arguments += $archivePath

& $pixiPackPath @arguments
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is & the powershell equivalent of exec?

prsabahrami and others added 5 commits October 11, 2024 18:09
Co-authored-by: Pavel Zwerschke <[email protected]>
Co-authored-by: Pavel Zwerschke <[email protected]>
Co-authored-by: Pavel Zwerschke <[email protected]>
Co-authored-by: Pavel Zwerschke <[email protected]>
Co-authored-by: Pavel Zwerschke <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we added tests for reproducible pixi-packs in #43

async fn test_reproducible_shasum(options: Options) {

could you extend that test with create_executable as well?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely.
I'll try to respond to all of the previous messages this weekend as well.
I have been extremely busy this week.

Copy link
Member

@pavelzw pavelzw Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! No worries, take your time 😄

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

Successfully merging this pull request may close these issues.

Self-extracting files
3 participants