Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* text=auto eol=lf

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Nice to have] Repo-wide * text=auto eol=lf will likely trigger a one-time renormalization diff for existing files, and may need exceptions for any files that must keep CRLF. Maybe worth a note in the PR or a follow-up git add --renormalize ..

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Only one tracked file had CRLF (vscode/workflow_settings_yaml.schema.json), and I renormalized it in this PR, so there's no follow-up diff. No files need to keep CRLF (there are no .bat/.cmd/.ps1 files in the repo).

105 changes: 105 additions & 0 deletions .github/workflows/test.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
name: Test

on:
pull_request:
push:
branches: [main]
workflow_dispatch:

permissions:
contents: read

jobs:
# Lint, unit and integration tests run on Cloud Build (cloudbuild-test.yaml); this job only
# builds the packages and the smoke test bundle that the Windows job needs.
build-linux:
name: Build packages and smoke test bundle (Linux)
runs-on: ubuntu-latest
timeout-minutes: 60
permissions:
contents: read
steps:
- name: Checkout code
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2

- name: Setup Bazel
uses: bazelbuild/setup-bazelisk@b39c379c82683a5f25d34f0d062761f62693e0b2 # v3.0.0

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not blocking here, but lets keep in mind that no Bazel cache is configured, so every PR does a cold build (60 min timeout). Let's consider adding a disk/repository cache (e.g. bazel-contrib/setup-bazel with disk-cache) to cut CI time.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, new issue will be created. For now the Linux job takes about a minute, so it isn't urgent.


# MODULE.bazel sets `use_home_npmrc = True`, so aspect_rules_js requires ~/.npmrc to exist.
# An empty file keeps the default public npm registry (Cloud Build writes its own).
- name: Create an empty ~/.npmrc
run: touch ~/.npmrc

- name: Build package tarballs and the Windows smoke test bundle
run: |
bazel build \
//packages/@dataform/cli:package_tar \
//packages/@dataform/core:package_tar \
//cli/tests/windows:smoke_test_bundle

- name: Upload CLI tarball
uses: actions/upload-artifact@4cec3d8aa04e39d1a68397de0c4cd6fb9dce8ec1 # v4.6.1
with:
name: dataform-cli-tarball
path: bazel-bin/packages/@dataform/cli/package.tar.gz

- name: Upload Core tarball
uses: actions/upload-artifact@4cec3d8aa04e39d1a68397de0c4cd6fb9dce8ec1 # v4.6.1
with:
name: dataform-core-tarball
path: bazel-bin/packages/@dataform/core/package.tar.gz

- name: Upload Windows smoke test bundle
uses: actions/upload-artifact@4cec3d8aa04e39d1a68397de0c4cd6fb9dce8ec1 # v4.6.1
with:
name: windows-smoke-test
path: bazel-bin/cli/tests/windows/smoke_test_bundle.js

test-windows:
Comment thread
rafal-hawrylak marked this conversation as resolved.
name: CLI smoke tests (Windows, Node ${{ matrix.node-version }})
needs: build-linux
runs-on: windows-latest
timeout-minutes: 30
strategy:
fail-fast: false
matrix:
# Oldest supported Node.js LTS and the current one (also used by the Bazel toolchain).
node-version: [22, 24]
permissions:
contents: read
steps:
- name: Setup Node.js
uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0
with:
node-version: ${{ matrix.node-version }}

- name: Download CLI tarball
uses: actions/download-artifact@cc203385981b70ca67e1cc392babf9cc229d5806 # v4.1.9
with:
name: dataform-cli-tarball
path: dist/cli

- name: Download Core tarball
uses: actions/download-artifact@cc203385981b70ca67e1cc392babf9cc229d5806 # v4.1.9
with:
name: dataform-core-tarball
path: dist/core

- name: Download Windows smoke test bundle
uses: actions/download-artifact@cc203385981b70ca67e1cc392babf9cc229d5806 # v4.1.9
with:
name: windows-smoke-test
path: dist/smoke

- name: Install @dataform/cli globally from tarball
run: npm install -g dist/cli/package.tar.gz

- name: Run Windows smoke tests in PowerShell
shell: powershell
run: |
node dist/smoke/smoke_test_bundle.js --shell powershell --core-tarball dist/core/package.tar.gz

- name: Run Windows smoke tests in cmd.exe
shell: cmd

Check warning on line 103 in .github/workflows/test.yaml

View workflow job for this annotation

GitHub Actions / zizmor-output

misfeature

test.yaml:103: usage of GitHub Actions misfeatures: shell defined here
run: |
node dist\smoke\smoke_test_bundle.js --shell cmd --core-tarball dist\core\package.tar.gz
2 changes: 1 addition & 1 deletion cli/api/commands/base_worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export abstract class BaseWorker<TResponse, TMessage = any> {
): Promise<TResponse> {
const forkScript = this.resolveScript();
const child = fork(forkScript, [], {
stdio: [0, 1, 2, "ipc", "pipe"],
stdio: [0, 1, 2, "ipc"],
Comment thread
apilaskowski marked this conversation as resolved.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Removing the extra "pipe" looks safe from the parent side (nothing reads child.stdio[4]). Could you confirm the worker/child script doesn't use fd 4 either?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed: nothing in cli/, common/ or core/ uses fd 4. The workers talk to the parent only over the IPC channel.

});

return new Promise((resolve, reject) => {
Expand Down
2 changes: 1 addition & 1 deletion cli/api/commands/compile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ export async function compile(
compiledGraph = dataform.CompiledGraph.create(decodedResult.compile.compiledGraph);

if (workflowSettingsDataformCoreVersion) {
fs.rmSync(temporaryProjectPath, { recursive: true });
fs.rmSync(temporaryProjectPath, { recursive: true, force: true, maxRetries: 3 });

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Nice to have] force: true will also mask genuine cleanup failures (not only the Windows EBUSY/EPERM races). Fine for a temp dir, just flagging; a debug log on failure might help diagnose leaks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

force only ignores the error when the path doesn't exist. EBUSY/EPERM and other errors are retried (maxRetries: 3) and then still thrown, so nothing is masked.

}

return compiledGraph;
Expand Down
13 changes: 11 additions & 2 deletions cli/commands/format_command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import { printError, printFormatFilesResult, printSuccess } from "df/cli/console
import { ICommand, INamedOption } from "df/cli/yargswrapper";
import { formatFile } from "df/sqlx/format";

const normalizeLineEndings = (text: string) => text.replace(/\r\n/g, "\n");
Comment thread
apilaskowski marked this conversation as resolved.

export interface IFormatArgs extends IProjectDirArgs, IActionsArgs {
ignoreJsFiles: boolean;
check: boolean;
Expand Down Expand Up @@ -50,7 +52,13 @@ export const formatCommand: ICommand<IFormatArgs> = {
actions = argv.actions;
}
const filenames = actions
.map((action: string) => glob.sync(action, { cwd: argv.projectDir }))
.map((action: string) =>
glob.sync(action, {
cwd: argv.projectDir,
// Treat "\" as a path separator only on Windows; on POSIX it remains a glob escape.
windowsPathsNoEscape: process.platform === "win32",
}),
)
.flat();

const isCheckMode = argv.check;
Expand All @@ -70,7 +78,8 @@ export const formatCommand: ICommand<IFormatArgs> = {
});
return {
filename,
needsFormatting: fileContent !== formattedContent,
needsFormatting:
normalizeLineEndings(fileContent) !== normalizeLineEndings(formattedContent),
};
} else {
// Normal formatting mode
Expand Down
26 changes: 26 additions & 0 deletions cli/index_project_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,5 +110,31 @@ modules.exports = {
const bufFromContents = Buffer.from(jsContents, "utf-8");
expect(bufFromContents.equals(bufFromFile)).equals(true);
});

test("format --check accepts correctly formatted files with CRLF line endings", async () => {
const projectDir = tmpDirFixture.createNewTmpDir();
await setupProject(tmpDirFixture, projectDir);
writeDefinitionFile(
projectDir,
"crlf.sqlx",
`
config { type: "table" }
SELECT 1 as test
`,
);
expect((await runCli("format", [projectDir])).exitCode).equals(0);

// Same formatted content, but with Windows line endings (e.g. a Git checkout with
// core.autocrlf=true).
const crlfFilePath = path.join(projectDir, "definitions", "crlf.sqlx");
const formattedContent = fs.readFileSync(crlfFilePath, "utf8");
expect(formattedContent).not.contains("\r\n");
fs.writeFileSync(crlfFilePath, formattedContent.replace(/\n/g, "\r\n"));

const checkResult = await runCli("format", [projectDir, "--check"]);

expect(checkResult.exitCode).equals(0);
expect(checkResult.stdout).contains("All files are formatted correctly");
});
});
});
22 changes: 22 additions & 0 deletions cli/tests/windows/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
load("//packages:index.bzl", "pkg_bundle")
load("//tools:ts_library.bzl", "ts_library")

package(default_visibility = ["//visibility:public"])

ts_library(
name = "smoke_test_lib",
srcs = ["smoke_test.ts"],
deps = [
"//:node_modules/@types/node",
],
)

# Self-contained CommonJS bundle (Node built-ins only) that the Windows CI runner executes with
# plain `node`; it is built on the Linux job and shipped as a workflow artifact.
pkg_bundle(
name = "smoke_test_bundle",
allow_node_builtins = True,
entry_point = "smoke_test.js",
externals = [],
deps = [":smoke_test_lib"],
)
Loading
Loading