ci[minor]: Start building on windows #6188
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
dosubot
bot
added
the
size:L
dosubot
bot
added
the
auto:improvement
bracesproul
commented
Jul 24, 2024
| import { Command } from "commander"; | ||
| import { rollup } from "rollup"; | ||
| import { rollup } from "@rollup/wasm-node"; |
There was a problem hiding this comment.
rollup does not work the same on windows, see here
bracesproul
commented
Jul 24, 2024
| @@ -27,6 +28,50 @@ async function asyncSpawn(command: string, args: string[]) { | |||
| }); | |||
| } | |||
|
|
|||
| const deleteFolderRecursive = async function (inputPath: string) { | |||
There was a problem hiding this comment.
Deleting directories on windows does not act the same as mac. Need this recursive func to manage deleting directories w/ contents.
bracesproul
commented
Jul 24, 2024
| @@ -16,6 +16,7 @@ async function asyncSpawn(command: string, args: string[]) { | |||
| ...process.env, | |||
| NODE_OPTIONS: "--max-old-space-size=4096", | |||
| }, | |||
| shell: true, | |||
There was a problem hiding this comment.
Required for windows
bracesproul
commented
Jul 24, 2024
Comment on lines
+665
to
+668
| if (process.platform === "win32") { | ||
| // windows, must resolve path with file:// | ||
| langchainConfigPath = `file:///${langchainConfigPath}`; | ||
| } |
There was a problem hiding this comment.
Can not import modules (langchain.config.js) on windows with the protocol d:
Full error:
Only URLs with a scheme in: file, data, and node are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'd:'
bracesproul
commented
Jul 24, 2024
| rimraf("dist"), | ||
| rimraf(".turbo"), | ||
| deleteFolderRecursive("dist").catch((e) => { | ||
| console.error("Error removing dist (pre && shouldGenMaps)"); |
There was a problem hiding this comment.
Added logs to make debugging a little easier
bracesproul
commented
Jul 24, 2024
Comment on lines
+726
to
+727
| // Required for cross-platform compatibility. | ||
| // Windows does not manage globs the same as Max/Linux when deleting directories. |
There was a problem hiding this comment.
Like the comment says...
bracesproul
commented
Jul 24, 2024
| @@ -43,3 +43,20 @@ jobs: | |||
| uses: | |||
| ./.github/workflows/test-exports.yml | |||
| secrets: inherit | |||
|
|
|||
| platform-compatibility: | |||
There was a problem hiding this comment.
Will now try and build @langchain/core on windows, macos and ubuntu.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes build when running on windows
Renames build script (npm deals with
:in a funny way for scripts)Added CI test for building
@langchain/corein macos, windows and ubuntu