Skip to content

push,build: Use streaming while packing the build context files #2995

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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

push,build: Use streaming while packing the build context files #2995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
88bb752
push,build: Use streaming while packing the build context files
thgreasi Feb 26, 2024
eb1ebdd
Deduplicate dependencies
thgreasi Oct 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions npm-shrinkwrap.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

58 changes: 39 additions & 19 deletions src/utils/compose_ts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { Flags } from '@oclif/core';
import type { BalenaSDK } from 'balena-sdk';
import type { TransposeOptions } from '@balena/compose/dist/emulate';
import type * as Dockerode from 'dockerode';
import { promises as fs } from 'fs';
import { promises as fs, createReadStream } from 'fs';
import * as yaml from 'js-yaml';
import * as _ from 'lodash';
import * as path from 'path';
Expand All @@ -30,6 +30,7 @@ import type {
import type * as MultiBuild from '@balena/compose/dist/multibuild';
import * as semver from 'semver';
import type { Duplex, Readable } from 'stream';
import { pipeline } from 'node:stream/promises';
import type { Pack } from 'tar-stream';
import { ExpectedError } from '../errors';
import type {
Expand Down Expand Up @@ -758,34 +759,53 @@ export async function tarDirectory(
const { toPosixPath } = (await import('@balena/compose/dist/multibuild'))
.PathUtils;

let readFile: (file: string) => Promise<Buffer>;
if (process.platform === 'win32') {
const { readFileWithEolConversion } = require('./eol-conversion');
readFile = (file) => readFileWithEolConversion(file, convertEol);
} else {
readFile = fs.readFile;
}
const getFileEolConverter =
process.platform === 'win32'
? (await import('./eol-conversion')).getFileEolConverter
: undefined;

const tar = await import('tar-stream');
const pack = tar.pack();
const serviceDirs = await getServiceDirsFromComposition(dir, composition);
const { filteredFileList, dockerignoreFiles } =
await filterFilesWithDockerignore(dir, multiDockerignore, serviceDirs);
printDockerignoreWarn(dockerignoreFiles, serviceDirs, multiDockerignore);
for (const fileStats of filteredFileList) {
pack.entry(
{
void (async () => {
Copy link
Member

@myarmolinsky myarmolinsky Oct 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why do we create an async function and void it here? Why not keep the for loop as it was before?

Copy link
Member Author

@thgreasi thgreasi Oct 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@myarmolinsky the whole point of this PR is to have this function return the stream w/o having to wait for the whole stream to be constructed (which is what the async-await part does). I'm still (like before) using async-await in here

  • b/c I'm using await for the param to the eolConverter (since I wasn't confident how I could correctly change it to be stream based)
  • for convenience, since it made it easier to (a)wait till all files are added, before we call await preFinalizeCallback & pack.finalize()

So the function returns right away the stream reference, and the void allows us to keep the async-await part working in the background and asynchronously add all files in the stream in order and call all required other functions/callbacks.

If we convert eolConverter to be stream based in a separate PR, we can then get rid of the void-async-await, and replace that by adding event listeners on every operation (tbh, I find the async-await code easier to read).

for (const fileStats of filteredFileList) {
const entryHeader = {
name: toPosixPath(fileStats.relPath),
mtime: fileStats.stats.mtime,
mode: fileStats.stats.mode,
size: fileStats.stats.size,
},
await readFile(fileStats.filePath),
);
}
if (preFinalizeCallback) {
await preFinalizeCallback(pack);
}
pack.finalize();
};

const eolConverter = getFileEolConverter?.(fileStats, convertEol);
if (eolConverter != null) {
pack.entry(
{
...entryHeader,
// When we need to convertEol we can't use streaming since pack.entry()
// only supports streaming when we do know the file size upfront, and
// we can't find the final size after the conversion upfront.
size: undefined,
},
// TODO: Consider using a tmp file/stream in a follow-up PR
// to allow streaming in this case as well. Since though we only
// convert eol for files up to LARGE_FILE_THRESHOLD (currently 10MB)
// the impact of such change is limited.
eolConverter(await fs.readFile(fileStats.filePath)),
);
} else {
const fileReadStream = createReadStream(fileStats.filePath);
const entry = pack.entry(entryHeader);
await pipeline(fileReadStream, entry);
}
}
if (preFinalizeCallback) {
await preFinalizeCallback(pack);
}
pack.finalize();
})();
return pack;
}

Expand Down
65 changes: 31 additions & 34 deletions src/utils/eol-conversion.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
* limitations under the License.
*/

import { promises as fs } from 'fs';
import Logger = require('./logger');
import type { FileStats } from './ignore';

const globalLogger = Logger.getLogger();

Expand Down Expand Up @@ -64,58 +64,55 @@ export function convertEolInPlace(buf: Buffer): Buffer {
}

/**
* Drop-in replacement for promisified fs.readFile(<string>)
* Attempts to convert EOLs from CRLF to LF for supported encodings,
* or otherwise logs warnings.
* Returns an EOL converter from CRLF to LF for supported encodings,
* or otherwise logs warnings. The result is either undefined or an
* async iterator, so that it can be used directly in pipeline().
* @param filepath
* @param convertEol When true, performs conversions, otherwise just warns.
*/
export async function readFileWithEolConversion(
filepath: string,
convertEol: boolean,
): Promise<Buffer> {
const fileBuffer = await fs.readFile(filepath);

export function getFileEolConverter(fileStats: FileStats, convertEol: boolean) {
// Skip processing of very large files
const fileStats = await fs.stat(filepath);
if (fileStats.size > LARGE_FILE_THRESHOLD) {
globalLogger.logWarn(`CRLF detection skipped for large file: ${filepath}`);
return fileBuffer;
if (fileStats.stats.size > LARGE_FILE_THRESHOLD) {
globalLogger.logWarn(
`CRLF detection skipped for large file: ${fileStats.filePath}`,
);
return;
}

// Analyse encoding
const encoding = detectEncoding(fileBuffer);
return function (fileBuffer: Buffer) {
// Analyse encoding
const encoding = detectEncoding(fileBuffer);

// Skip further processing of non-convertible encodings
if (!CONVERTIBLE_ENCODINGS.includes(encoding)) {
return fileBuffer;
}
// Skip further processing of non-convertible encodings
if (!CONVERTIBLE_ENCODINGS.includes(encoding)) {
return fileBuffer;
}

// Skip further processing of files that don't contain CRLF
if (!fileBuffer.includes('\r\n')) {
return fileBuffer;
}
// Skip further processing of files that don't contain CRLF
if (!fileBuffer.includes('\r\n')) {
return fileBuffer;
}

if (convertEol) {
// Convert CRLF->LF
globalLogger.logInfo(
`Converting line endings CRLF -> LF for file: ${filepath}`,
);
if (convertEol) {
// Convert CRLF->LF
globalLogger.logInfo(
`Converting line endings CRLF -> LF for file: ${fileStats.filePath}`,
);

return convertEolInPlace(fileBuffer);
}

return convertEolInPlace(fileBuffer);
} else {
// Immediate warning
globalLogger.logWarn(
`CRLF (Windows) line endings detected in file: ${filepath}`,
`CRLF (Windows) line endings detected in file: ${fileStats.filePath}`,
);
// And summary warning later
globalLogger.deferredLog(
'Windows-format line endings were detected in some files, but were not converted due to `--noconvert-eol` option.',
Logger.Level.WARN,
);

return fileBuffer;
}
};
}

/**
Expand Down
Loading