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(attest): windows local dev #1129

Closed
wants to merge 1 commit into from

Conversation

Dimava
Copy link
Contributor

@Dimava Dimava commented Sep 15, 2024

No description provided.

Copy link
Member

@ssalbdivad ssalbdivad left a comment

Choose a reason for hiding this comment

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

I don't really understand this passes CI in Windows.

The concern around symlinks not working on Windows was only in old versions, right?

@ssalbdivad ssalbdivad changed the title make attest work on Windows fix(attest): windows local dev Sep 15, 2024
@Dimava
Copy link
Contributor Author

Dimava commented Sep 16, 2024

Symlinks require sudo on Windows, unlike NTFS Junctions
(unleast you use the Dev Drive feature I think like 50% of devs don't know about and 80% don't use)

    // Directory symlink on Windows. This comes with the following caveats:
    // * NTFS Junctions cannot be relative.
    // * NTFS Junctions MUST be directories.
    // * NTFS Junctions must be on the same file system.
    // * Most products CANNOT detect a directory is a Junction:
    //    This has the side effect of possibly having a whole directory
    //    deleted when a product is deleting the Junction directory.
    //    For example, IntelliJ product lines will delete the entire
    //    contents of the TARGET directory because the product does not
    //    realize it's a symlink as the JVM and Node return false for isSymlink.

See gulpjs/vinyl-fs#210 gulpjs/vinyl-fs#231
On linux the link-type arg is ignored as jsdoc says

@Dimava
Copy link
Contributor Author

Dimava commented Sep 16, 2024

🤔
I think you should use async imports, then you don't need all that directory linking and this commit I was going to make

 	const nodeModules = join(assertPackageRoot(process.cwd()), "node_modules")
 	const tsPrimaryPath = join(nodeModules, "typescript")
 	const tsTemporaryPath = join(nodeModules, "typescript-temp")
-	if (existsSync(tsTemporaryPath)) unlinkSync(tsTemporaryPath)
+	if (existsSync(tsTemporaryPath)) {
+		// if the runner was killed while this function was running, the changes won't be reverted
+		// then if both exist, the current "primary" one is probably a symlink to a secondary one
+		if (existsSync(tsPrimaryPath)) unlinkSync(tsPrimaryPath)
+		else unlinkSync(tsTemporaryPath)
+	}
 	if (existsSync(tsPrimaryPath)) renameSync(tsPrimaryPath, tsTemporaryPath)

const ts = (await import(config.tsPath ?? 'ts')).default

aaand that's it?
If you don't parallelize it you may even do that within the thread

@ssalbdivad
Copy link
Member

Closing this but if there are some improvements for windows local dev feel free to include them in your Bun PR.

@ssalbdivad ssalbdivad closed this Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done (merged or closed)
Development

Successfully merging this pull request may close these issues.

2 participants