-
Notifications
You must be signed in to change notification settings - Fork 592
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
Rush should create .tgz files instead of package.json for temp_modules #324
Conversation
// Example: "C:\MyRepo\common\temp\projects\my-project-2\package.json" | ||
const tempPackageJsonFilename: string = path.join(tempProjectFolder, RushConstants.packageJsonFilename); | ||
|
||
// Example: "C:\MyRepo\common\temp\projects\my-project-2.gzip" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gzip [](start = 63, length = 4)
*.tgz? #Resolved
|
||
// ensure the folder we are about to extract into is clean | ||
fsx.removeSync(extractedFolder); | ||
fsx.mkdirpSync(extractedFolder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails intermittently due the virus scanner.
See Utilities.createFolderWithRetry() for more info #Resolved
const oldBuffer: Buffer = fsx.readFileSync(extractedPackageJsonFilename); | ||
const newBuffer: Buffer = new Buffer(JsonFile.normalize(tempPackageJson)); | ||
|
||
if (Buffer.compare(oldBuffer, newBuffer) === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Buffer.compare(oldBuffer, newBuffer) === 0) [](start = 13, length = 44)
What about encodings? #WontFix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I borrowed this code from JsonFile, which you wrote. I suggest we fix this in all places
In reply to: 133606562 [](ancestors = 133606562)
if (shouldOverwrite) { | ||
// ensure the folder we are about to zip is clean | ||
fsx.removeSync(tempProjectFolder); | ||
fsx.mkdirpSync(tempProjectFolder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue with virus scanner #Resolved
JsonFile.saveJsonFile(tempPackageJson, tempPackageJsonFilename); | ||
|
||
// create the new tarball, this overwrites the existing one | ||
tar.create({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create [](start = 12, length = 6)
I would try once more to see if you can create the tarball without writing any files to disk. Although the code will be a little more complicated, the runtime behavior will be faster and less likely to run into weird file locking issues. #WontFix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also measure how long it takes to make 70 tarballs in sp-client
In reply to: 133607227 [](ancestors = 133607227)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently the library I am using (which is the most-used tar library, written by none other than Isaac S. III) doesn't support in-memory creation. I will do a perf test in sp-client though.
In reply to: 133607247 [](ancestors = 133607247,133607227)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickpape-msft archiver --> tar-stream FYI #WontFix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adds .33 seconds to rush install/generate time
In reply to: 133798619 [](ancestors = 133798619,133607247,133607227)
const tempProjectFolder: string = path.dirname(rushProject.tempPackageJsonFilename); | ||
fsx.mkdirsSync(tempProjectFolder); | ||
// Example: "my-project-2" | ||
const unscopedTempProjectName: string = path.basename(rushProject.tempPackageTarballFilename, '.tgz'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
path.basename(rushProject.tempPackageTarballFilename, '.tgz'); [](start = 46, length = 62)
This is a weird design. On line 393 you use "path.dirname(rushProject.tempPackageTarballFilename)" to calculate the same thing.
Why not just make "unscopedTempProjectName" your API property (e.g. called "tempProjectUniqueName"), and then construct the other derivatives from that. I'd argue that these derivatives are all internal guts of InstallManager, not something that rush-lib consumers should be privy to.
installCommonModules() is a slightly tricky case, but you could just make a private helper function getTarballPath(project) #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this suggestion. Done. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rush needs to make
file:
references to tgz files. NPM has decided to deprecate the behavior of thefile:./...
specified, which used to treat the folder as a project that needed to be installed. Instead,file:./...
references now just create a symlink, unless one is referencing a.tgz
file. PNPM has also decided to use this behavior.We should merge this PR to:
See:
npm/npm#15900
and
pnpm/pnpm#772