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

fix(cli): handle patterns correctly on Windows #10430

Merged
merged 1 commit into from
Jun 22, 2024
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
32 changes: 32 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,38 @@ jobs:
run: npm run test:cov
if: ${{ !cancelled() }}

cli-unit-tests-win:
name: CLI (Windows)
runs-on: windows-latest
defaults:
run:
working-directory: ./cli

steps:
- name: Checkout code
uses: actions/checkout@v4

- name: Setup Node
uses: actions/setup-node@v4
with:
node-version: 20

- name: Setup typescript-sdk
run: npm ci && npm run build
working-directory: ./open-api/typescript-sdk

- name: Install deps
run: npm ci

# Skip linter & formatter in Windows test.
- name: Run tsc
run: npm run check
if: ${{ !cancelled() }}

- name: Run unit tests & coverage
run: npm run test:cov
if: ${{ !cancelled() }}

web-unit-tests:
name: Web
runs-on: ubuntu-latest
Expand Down
14 changes: 11 additions & 3 deletions cli/src/utils.spec.ts
Copy link
Member

Choose a reason for hiding this comment

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

There's a few changes in this test that look odd to me, can you explain what's going on here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Line #265: On the Windows platform, / cannot be treated as expected in fast-glob. I've filed an issue here mrmlnc/fast-glob#447.

Line #286 & #294: Comparing path strings directly will lead to some false positive cases. For example, on Windows, a path can be written as D:\Something\Like\This while \something\like\this on Linux. So It should be better to compare what they've actually read in the file system. To achieve this, I'm letting each file contain unique contents (their paths) so that it can be used for comparison later. A good thing to note is that we are still comparing their paths and this is friendly when debugging.

Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import mockfs from 'mock-fs';
import { readFileSync } from 'node:fs';
import { CrawlOptions, crawl } from 'src/utils';

interface Test {
Expand All @@ -9,6 +10,10 @@ interface Test {

const cwd = process.cwd();

const readContent = (path: string) => {
return readFileSync(path).toString();
};

const extensions = [
'.jpg',
'.jpeg',
Expand Down Expand Up @@ -256,7 +261,8 @@ const tests: Test[] = [
{
test: 'should support ignoring absolute paths',
options: {
pathsToCrawl: ['/'],
// Currently, fast-glob has some caveat when dealing with `/`.
fky2015 marked this conversation as resolved.
Show resolved Hide resolved
pathsToCrawl: ['/*s'],
recursive: true,
exclusionPattern: '/images/**',
},
Expand All @@ -276,14 +282,16 @@ describe('crawl', () => {
describe('crawl', () => {
for (const { test, options, files } of tests) {
it(test, async () => {
mockfs(Object.fromEntries(Object.keys(files).map((file) => [file, ''])));
// The file contents is the same as the path.
mockfs(Object.fromEntries(Object.keys(files).map((file) => [file, file])));

const actual = await crawl({ ...options, extensions });
const expected = Object.entries(files)
.filter((entry) => entry[1])
.map(([file]) => file);

expect(actual.sort()).toEqual(expected.sort());
// Compare file's content instead of path since a file can be represent in multiple ways.
expect(actual.map((path) => readContent(path)).sort()).toEqual(expected.sort());
});
}
});
Expand Down
12 changes: 9 additions & 3 deletions cli/src/utils.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { getMyUser, init, isHttpError } from '@immich/sdk';
import { glob } from 'fast-glob';
import { convertPathToPattern, glob } from 'fast-glob';
import { createHash } from 'node:crypto';
import { createReadStream } from 'node:fs';
import { readFile, stat, writeFile } from 'node:fs/promises';
import { platform } from 'node:os';
import { join, resolve } from 'node:path';
import yaml from 'yaml';

Expand Down Expand Up @@ -106,6 +107,11 @@ export interface CrawlOptions {
exclusionPattern?: string;
extensions: string[];
}

const convertPathToPatternOnWin = (path: string) => {
return platform() === 'win32' ? convertPathToPattern(path) : path;
};
Comment on lines +111 to +113
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be conditional to the platform, would there be any issues with just always converting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, convertPathToPattern will change the matching behavior on *nix platform:

/photo* will become /photo\* thus the asterisk will lose its effect.

Copy link
Member

Choose a reason for hiding this comment

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

But it doesn't change the behaviour on windows? If so that sounds weird, do you know why it is like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it doesn't change the behaviour on Windows.

From my understanding, convertPathToPattern is a function especially used for converting a Windows path to fast-glob search pattern. It is meaningless to use this function if your inputs are a valid POSIX path.


export const crawl = async (options: CrawlOptions): Promise<string[]> => {
const { extensions: extensionsWithPeriod, recursive, pathsToCrawl, exclusionPattern, includeHidden } = options;
const extensions = extensionsWithPeriod.map((extension) => extension.replace('.', ''));
Expand All @@ -124,11 +130,11 @@ export const crawl = async (options: CrawlOptions): Promise<string[]> => {
if (stats.isFile() || stats.isSymbolicLink()) {
crawledFiles.push(absolutePath);
} else {
patterns.push(absolutePath);
patterns.push(convertPathToPatternOnWin(absolutePath));
}
} catch (error: any) {
if (error.code === 'ENOENT') {
patterns.push(currentPath);
patterns.push(convertPathToPatternOnWin(currentPath));
} else {
throw error;
}
Expand Down
1 change: 1 addition & 0 deletions cli/vite.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { defineConfig } from 'vite';
import tsconfigPaths from 'vite-tsconfig-paths';

export default defineConfig({
resolve: { alias: { src: '/src' } },
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

@fky2015 fky2015 Jun 20, 2024

Choose a reason for hiding this comment

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

I followed cli/README.md and filed to build the project without this change to the config (on Windows).

error during build:
[vite]: Rollup failed to resolve import "src/commands/asset" from "D:\code\immich\cli\src\index.ts".

image

Generally speaking, I think this change will improve the compatibility of dev configs.

build: {
rollupOptions: {
input: 'src/index.ts',
Expand Down
Loading