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

Keep Source #669

Merged
merged 13 commits into from
May 14, 2020
Merged

Keep Source #669

merged 13 commits into from
May 14, 2020

Conversation

Stratus3D
Copy link
Member

@Stratus3D Stratus3D commented Mar 7, 2020

Summary

  • Add support for a download callback script in plugins. This script will eventually be required.
  • Add support for a always_keep_download configuration option.
  • Add support for a --keep-download flag for the install command.

Fixes: #74

This PR addresses the oldest open issue in our issue tracker! 🎉

Other Information

This PR is the first of two PRs to add this keep source/download management functionality. The next PR will add three new commands to asdf:

  asdf download <name> <version>           Download a specific version of a package
  asdf download list [<name>]              List all downloads, optionally filter by package name
  asdf download rm <name> <version>        Remove the download for a specific version of a package

@Stratus3D Stratus3D added the WIP label Mar 7, 2020
@jthegedus
Copy link
Contributor

jthegedus commented Mar 7, 2020

Is there an intent to skip the download if the specific version is already present?

@jthegedus
Copy link
Contributor

Another question, what is the plan for a plugin that has a bin/download script but the user is on an older version of asdf that is unaware of the download script? Should plugin authors just ask them to update or do something more elaborate?

@Stratus3D
Copy link
Member Author

@jthegedus yes that is the intent. Although that reminds me I also may implement a --force-redownload flag for the install command as well. Although I suppose the new asdf download rm <tool> <version> command (not yet added to this PR) would do the same thing.

@Stratus3D
Copy link
Member Author

Another question, what is the plan for a plugin that has a bin/download script but the user is on an older version of asdf that is unaware of the download script?

Good question. I didn't think about that. I was implementing this as something that would be backward compatible with old plugins. I guess this would not result in new plugins working with older versions of asdf.

@Stratus3D
Copy link
Member Author

Actually, plugins can be backwards compatible with the current version of asdf. All the install script needs to do is check for the presence of the ASDF_DOWNLOAD_PATH. If it is present assume the source has already been downloaded to that directory. Otherwise assume an older version of asdf and download the source to a temp directory before installing.

@Stratus3D
Copy link
Member Author

I am going to update my asdf-lua plugin with support for this as a test.

@jthegedus
Copy link
Contributor

jthegedus commented Mar 17, 2020

Is this what you meant @Stratus3D ? Using in asdf-firebase and asdf-gcloud

# bin/install
    if [ -z "${ASDF_DOWNLOAD_PATH:-}" ]; then
        tmp_download_dir=$(mktemp -d -t 'asdf_${plugin_name}_XXXXXX')
        trap 'rm -rf "${tmp_download_dir}"' EXIT
        printf "run download script for older versions of asdf\\n"
        export ASDF_DOWNLOAD_PATH="${tmp_download_dir}"

        # download
        bash "$(dirname "$0")/download"
    fi

@Stratus3D
Copy link
Member Author

@jthegedus yes. Exactly.

Hoping to finish this PR this week.

@Stratus3D Stratus3D requested a review from a team as a code owner May 4, 2020 15:15
@Stratus3D Stratus3D removed the WIP label May 8, 2020
@Stratus3D
Copy link
Member Author

This PR is finally ready for review @asdf-vm/core !

@jthegedus
Copy link
Contributor

Awesome, I'll give it a test with my plugins this weekend 💯

@Stratus3D
Copy link
Member Author

Had a chance to test this yet @jthegedus ?

@jthegedus
Copy link
Contributor

Sorry mate, not yet. Will try do so tomorrow. Been slammed with work 😫

@Stratus3D Stratus3D changed the title WIP: Keep Source Keep Source May 13, 2020
@jthegedus
Copy link
Contributor

jthegedus commented May 14, 2020

Tested this out, works a treat 👌

One more question though, with asdf plugin remove <tool>, should the default behaviour remove the cached downloads or not? Perhaps a flag for asdf plugin remove <tool> --remove-downloads to cleanup after a plugin? Or the inverse asdf plugin remove <tool> --keep-downloads? Or make it remove the download dir of the plugin depending on the value of the keep_downloads flag?

@Stratus3D
Copy link
Member Author

@jthegedus good question. At first I was thinking not remove the downloads directory when the plugin is uninstalled. But I think problems could arise if a different plugin with the same name were installed. The downloaded files might differ between plugins and cause problems. I think it's best to avoid this by always removing downloads when a plugin is removed.

@Stratus3D Stratus3D merged commit a7252e6 into master May 14, 2020
@Stratus3D Stratus3D deleted the tb/keep-source branch May 14, 2020 13:24
@jthegedus
Copy link
Contributor

I think problems could arise if a different plugin with the same name were installed

Perhaps making the download path be the GitHub username and repo name, like

https://github.com/jthegedus/asdf-firebase

to

jthegedus-asdf-firebase

would be another option.

Removing downloads when plugin is removed is easier

@Stratus3D
Copy link
Member Author

That does get around the issue of the downloads being available to a plugin that works differently, but it would make the asdf downloads command that I am going to add more complicated. The API I have in mind is:

asdf download-list [<tool>]
asdf download <tool> <version>
asdf download-rm [<tool> <version>],[--all]

If we had to take into account the tool name and some additional plugin identifier like username, these commands would be more complicated. Maybe it is worth it, I am not sure.

@Stratus3D
Copy link
Member Author

With the way I use asdf, I think I've removed plugins maybe 3-4 times over the last 4 years of using it. Others may use it differently, but to me it doesn't seem to be a common action.

@jthegedus
Copy link
Contributor

With the way I use asdf, I think I've removed plugins maybe 3-4 times over the last 4 years of using it. Others may use it differently, but to me it doesn't seem to be a common action.

I would agree that it is not common.

asdf downloads...

Do we need these additional commands? I think removing all downloads on plugin removal as the default behaviour would be enough no? I get list and rm, but why have asdf download <tool> <version>?

@Stratus3D
Copy link
Member Author

Do we need these additional commands? I think removing all downloads on plugin removal as the default behaviour would be enough no? I get list and rm, but why have asdf download ?

Good question. I assumed it would make sense to allow the user to download a version first before installing if they wanted to apply manual patches the source code before installing. Having an asdf download <tool> <version> command makes that easier. asdf download ls would show the paths to downloads if the user wanted to modify source code at a later time. And asdf download rm would make it easy to remove files from a download that went bad (e.g. download succeeded but a plugin or server bug results in unusable files being downloaded). Maybe an asdf download command isn't all that important. I'm open to not having it if you think that would be better.

@jthegedus jthegedus added this to the v0.8.x milestone Aug 20, 2020
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.

[Proposal] Option "-k" to keep source
2 participants