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

Prepend directory to the binary tarball #6701

Merged
merged 2 commits into from Oct 22, 2022
Merged

Conversation

nibon7
Copy link
Contributor

@nibon7 nibon7 commented Oct 11, 2022

Description

According to https://www.gnu.org/software/tar/manual/html_section/transform.html, use --transform parameter to prepend directory to each file name.

GNU tar is not available on Mac by default, use a common way.

Closes #6676

Screenshot from 2022-10-11 17-24-53

Tests

Make sure you've done the following:

  • Add tests that cover your changes, either in the command examples, the crate/tests folder, or in the /tests folder.
  • Try to think about corner cases and various ways how your changes could break. Cover them with tests.
  • If adding tests is not possible, please document in the PR body a minimal example with steps on how to reproduce so one can verify your change works.

Make sure you've run and fixed any issues with these commands:

  • cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
  • cargo clippy --workspace --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
  • cargo test --workspace --features=extra to check that all the tests pass

According to https://www.gnu.org/software/tar/manual/html_section/transform.html, use
`--transform` parameter to prepend directory to each file name.

Closes nushell#6676
@nibon7 nibon7 marked this pull request as draft October 11, 2022 05:32
@sholderbach
Copy link
Member

Do we have any consumers of our tarballs that might need adjustment with this change?

@nibon7
Copy link
Contributor Author

nibon7 commented Oct 11, 2022

Do we have any consumers of our tarballs that might need adjustment with this change?

Nope, as @christianhujer said, the tar archive usually contains a directory.

@nibon7 nibon7 marked this pull request as ready for review October 11, 2022 10:09
@sholderbach
Copy link
Member

I am wondering if we need to change things for downstream tools or package distributors that download the nu binaries from the GitHub release.
We would have to make sure they can deal with this change correctly (and if they support version pinning to older versions, things don't break with a change to the recipe)

Examples:
https://github.com/Yethal/ansible-role-nushell
https://github.com/marketplace/actions/setup-nu

Dragging in @Yethal and @hustcer for comment

@hustcer
Copy link
Contributor

hustcer commented Oct 11, 2022

Setup-nu Action may not work well with the new release workflow, I will test and fix it recently

@Yethal
Copy link
Contributor

Yethal commented Oct 13, 2022

No change from my perspective. Ansible role already searches recursively to find the files. With that said, the only reason I'm doing it in the first place is because tarball does not have the directory prepended to it.

@fdncred
Copy link
Collaborator

fdncred commented Oct 15, 2022

I'm nervous about this one because all downstream consumers might need to change something.

@hustcer
Copy link
Contributor

hustcer commented Oct 18, 2022

Feel free to merge this PR, I will fix setup-nu action if it fails for the new release tarball

@Yethal
Copy link
Contributor

Yethal commented Oct 18, 2022

@fdncred maybe post about this change on discord/twitter/etc to reach other consumers, other than me and hustcer there are probably people maintaining packages in third party repos such as aur or pacstall

@rgwood
Copy link
Contributor

rgwood commented Oct 20, 2022

This seems like a good idea to me.

In general, Linux package managers are going to be building Nu from source and won't be affected by this. Homebrew builds from source too.

@Yethal
Copy link
Contributor

Yethal commented Oct 21, 2022

Package managers yes, but places such as pacstall, aur, homebrew etc that automate the process of downloading the release from github might be affected

@rgwood
Copy link
Contributor

rgwood commented Oct 21, 2022

Homebrew builds Nu from scratch. I believe AUR does too. I understand that some tools might use our binaries but I suspect the number of such tools is quite small.

@fdncred
Copy link
Collaborator

fdncred commented Oct 22, 2022

let's move forward with this. I just hope it doesn't come back to bite us on Nov 8th release. Thanks!

@fdncred fdncred merged commit 4fdf5c6 into nushell:main Oct 22, 2022
@nibon7 nibon7 deleted the release_tarball branch October 23, 2022 14:48
@hustcer
Copy link
Contributor

hustcer commented Nov 9, 2022

setup-nu@v3 supports latest nu v0.71.0

@rgwood
Copy link
Contributor

rgwood commented Nov 10, 2022

Looks like this ended up breaking virtualenv. Hopefully they can work around it. pypa/virtualenv#2442 (comment)

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.

tar archive should contain a directory
6 participants