Skip to content

Improve tgz file handling #7229

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

Open
wants to merge 9 commits into
base: release-2.1
Choose a base branch
from
Open

Conversation

live627
Copy link
Contributor

@live627 live627 commented Jan 9, 2022

@live627 live627 added this to the 2.1.0 milestone Jan 9, 2022
@live627 live627 requested a review from jdarwood007 January 9, 2022 22:32
@jdarwood007
Copy link
Member

jdarwood007 commented Jan 9, 2022

Is this compatible with #7228 or an alternative fix?

@live627
Copy link
Contributor Author

live627 commented Jan 10, 2022

This is alternative fix

@sbulen
Copy link
Contributor

sbulen commented Jan 10, 2022

@live627 - two things:

  • It doesn't fix the initial problem, I am still seeing the same/similar issues when the OSX files are present, e.g., WSOD in the Package Manager:
    image

I am also seeing issues trying to analyze a package for the cust site - slightly different, though, it cannot find package-info.xml at all.

  • I am getting thousands of errors in the error log:
    2: Undefined array key "filename" in Subs-Package, various lines, including 210, 206, 186, 178

I used the same test file we were using on the cust site with the OSX files. (Since I don't have a mac I need to use @jdarwood007 's example...)

@sbulen
Copy link
Contributor

sbulen commented Jan 10, 2022

I saw you pushed some commits while I was writing up my earlier test results.

I just retested, and all the issues reported above appear to have been addressed.

I must admit, though, that rewrites of stable code always make me nervous... One of my most repeated complaints with PRs here...

@sbulen
Copy link
Contributor

sbulen commented Jan 10, 2022

(When the destination is null, that is a request for a list of files in the archive.)

@jdarwood007
Copy link
Member

If both fix the same thing, then I guess its a mater of choice.

This one feels like it may be more complete in trying to handle the tar file according to the RFC.

However we are late in 2.1 stages. I see it also added hints. While I like type hints, we have to be very careful of their usage in 2.1, because of so many places we could possibly break it. This PR also may need additional testing for open_basedir restriction testing, which the $destination !== null checks fixed by preventing us from doing file checks on directories that would start from a path most likely outside of the allowed dirs.

In all honesty, I think #7228 should be merged for 2.1. This one should go into the future because it is more complete into the RFC spec (as I see it handling some additional types other than files). @Sesquipedalian may want to make that decision though.

@live627
Copy link
Contributor Author

live627 commented Jan 18, 2022

Testing this one is pretty much the same as #7180 (comment)

@sbulen
Copy link
Contributor

sbulen commented Jan 21, 2022

@live627 - I'm not sure this is a hard requirement for 2.1.0?

@live627 live627 removed this from the 2.1.0 milestone Jan 21, 2022
@live627
Copy link
Contributor Author

live627 commented Jan 21, 2022

Not important enough to hold up the release. The excising code works fine with the ubiquitous ustar format, just not the "newer" pax format.

I'll let @Sesquipedalian assign a new milestone as he sees fit.

@Sesquipedalian Sesquipedalian added this to the The future milestone Jan 24, 2022
@Sesquipedalian
Copy link
Member

Sesquipedalian commented Jan 24, 2022

I'd rather merge the full fix. However, there's really no hurry for dealing with this issue. I've been making SMF compatible tar.gz files on my Mac for years; it's just a matter of setting the right command line switches. Anyway, we can take the time to test this PR as thoroughly as we like after 2.1.0 is out.

@jdarwood007
Copy link
Member

FYI, this didn't get merged into 2.1.0. Can we do the #7228 for 2.1.x?

@live627
Copy link
Contributor Author

live627 commented Jul 9, 2023

The speedup mentioned in the latest commit is 0.2 seconds with a package directory filled with 89 objects; package directories, php files, package files.

Signed-off-by: John Rayes <[email protected]>
@@ -1587,25 +1587,37 @@ function getPackages()

$installed_mods = loadInstalledPackages();

$packagesdir='C:\Users\John\nf\df';
Copy link
Member

Choose a reason for hiding this comment

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

Derp

if ($file_info['filename'][-1] !== '/')
$file_info['filename'] .= '/';
}
elseif ($file_info['type'] === 'L')
Copy link
Member

Choose a reason for hiding this comment

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

Since your updating these. Can you document the types heres. Still confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package manager won't parse some packages generated on OSX
4 participants