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

Refactor to support generic Python "zipapps" #221

Merged
10 commits merged into from
Apr 23, 2018

Conversation

warsaw
Copy link
Contributor

@warsaw warsaw commented Apr 12, 2018

pex is just one implementation of a Python "zipapp". Users might want to support other types of zipapps, so this branch refactors pex support so as to add a generic ZipappExtension and related classes. Then it reimplements pex support on top of that.

This should not change existing pex support, for either fat or thin pexes. It also keeps the build.gradle settings for selecting thin vs fat pexes, but it also allows for a more generic specification. E.g.

python {
    pex {
        isFat = false
    }
}

(i.e. allows for isFat and fatPex for backward compatibility)

@warsaw
Copy link
Contributor Author

warsaw commented Apr 12, 2018

Since this is a refactoring branch I didn't add any new tests, but the existing tests should still pass and provide a similar level of coverage.

import java.io.File;


public class ZipappExtension {
Copy link

Choose a reason for hiding this comment

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

I think this would should be an interface vs a class. We can make an abstract class to hold some helpers if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made ZipappExtension a class to hold the common fields cache and isFat which are used in both the pex extension subclass, and the (currently only available in a separate non-public extension) shiv extension subclass. If we make ZipappExtension an interface, where will we put cache and isFat? Also, I think this class is more about providing common functionality for subclasses rather than defining a useful interface that other classes should implement. E.g., it's all about getting and setting the cache directory and the "fatness" of the artifacts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kopy07 Fair enough. I think now that we have the cache property sorted out, I agree with you about making ZipappExtension an interface. The next push will do that.

import java.util.Map;


public class ThinZipappGenerator implements ZipappGenerator {
Copy link

Choose a reason for hiding this comment

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

Is this class usable directly, or will people have to extend it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They will have to extend it to implement the buildEntryPoints() method. See ThinPexGenerator. This is essentially a refactor of the original ThinPexGenerator class for commonality with the internal thin shiv generator class.

public class PexExtension {

private File pexCache;
private boolean fatPex = OperatingSystem.current().isWindows(); //Defaulting to fat pex's on windows
Copy link

Choose a reason for hiding this comment

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

Where is this captured now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ZipappExtension class referenced above :)

@ghost ghost self-assigned this Apr 18, 2018
@ghost ghost requested a review from zvezdan April 18, 2018 22:18
@warsaw
Copy link
Contributor Author

warsaw commented Apr 19, 2018

@Kopy07 The latest change turns ZipappExtension into an interface and adds some @Deprecated annotations as discussed off-line.

@ghost ghost merged commit 5f446bd into linkedin:master Apr 23, 2018
@warsaw warsaw deleted the zipapp-refactor branch April 2, 2019 23:21
This pull request was closed.
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.

1 participant