From 3f10d5250a9a1e5a9b9ec726c9a0611126f2be50 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Mon, 15 Apr 2024 13:06:30 -0700 Subject: [PATCH 1/8] [bun:sqlite] Support `sqlite3_file_control`, better closing behavior, implement Disposable (#10262) * [bun:sqlite] Support `sqlite_file_control`, better closing behavior, support `using` statements * docs+flaky test * Simplify the implementation --- docs/api/sqlite.md | 54 +++- packages/bun-types/sqlite.d.ts | 218 ++++++++++++- src/bun.js/bindings/sqlite/JSSQLStatement.cpp | 300 ++++++++++-------- src/bun.js/bindings/sqlite/lazy_sqlite3.h | 8 + src/js/bun/sqlite.ts | 75 ++++- test/js/bun/sqlite/sqlite.test.js | 108 ++++++- 6 files changed, 610 insertions(+), 153 deletions(-) diff --git a/docs/api/sqlite.md b/docs/api/sqlite.md index ddc405fb02571..04ab1bbf4d63d 100644 --- a/docs/api/sqlite.md +++ b/docs/api/sqlite.md @@ -62,7 +62,7 @@ const db = new Database("mydb.sqlite", { create: true }); You can also use an import attribute to load a database. ```ts -import db from "./mydb.sqlite" with {"type": "sqlite"}; +import db from "./mydb.sqlite" with { "type": "sqlite" }; console.log(db.query("select * from users LIMIT 1").get()); ``` @@ -74,16 +74,39 @@ import { Database } from "bun:sqlite"; const db = new Database("./mydb.sqlite"); ``` -### `.close()` +### `.close(throwOnError: boolean = false)` -To close a database: +To close a database connection, but allow existing queries to finish, call `.close(false)`: ```ts const db = new Database(); -db.close(); +// ... do stuff +db.close(false); ``` -Note: `close()` is called automatically when the database is garbage collected. It is safe to call multiple times but has no effect after the first. +To close the database and throw an error if there are any pending queries, call `.close(true)`: + +```ts +const db = new Database(); +// ... do stuff +db.close(true); +``` + +Note: `close(false)` is called automatically when the database is garbage collected. It is safe to call multiple times but has no effect after the first. + +### `using` statement + +You can use the `using` statement to ensure that a database connection is closed when the `using` block is exited. + +```ts +import { Database } from "bun:sqlite"; + +{ + using db = new Database("mydb.sqlite"); + using query = db.query("select 'Hello world' as message;"); + console.log(query.get()); // => { message: "Hello world" } +} +``` ### `.serialize()` @@ -128,6 +151,8 @@ db.exec("PRAGMA journal_mode = WAL;"); {% details summary="What is WAL mode" %} In WAL mode, writes to the database are written directly to a separate file called the "WAL file" (write-ahead log). This file will be later integrated into the main database file. Think of it as a buffer for pending writes. Refer to the [SQLite docs](https://www.sqlite.org/wal.html) for a more detailed overview. + +On macOS, WAL files may be persistent by default. This is not a bug, it is how macOS configured the system version of SQLite. {% /details %} ## Statements @@ -387,6 +412,25 @@ db.loadExtension("myext"); {% /details %} +### .fileControl(cmd: number, value: any) + +To use the advanced `sqlite3_file_control` API, call `.fileControl(cmd, value)` on your `Database` instance. + +```ts +import { Database, constants } from "bun:sqlite"; + +const db = new Database(); +// Ensure WAL mode is NOT persistent +// this prevents wal files from lingering after the database is closed +db.fileControl(constants.SQLITE_FCNTL_PERSIST_WAL, 0); +``` + +`value` can be: + +- `number` +- `TypedArray` +- `undefined` or `null` + ## Reference ```ts diff --git a/packages/bun-types/sqlite.d.ts b/packages/bun-types/sqlite.d.ts index f0ef907e82a47..1f2fd7ff5db1b 100644 --- a/packages/bun-types/sqlite.d.ts +++ b/packages/bun-types/sqlite.d.ts @@ -24,7 +24,7 @@ * | `null` | `NULL` | */ declare module "bun:sqlite" { - export class Database { + export class Database implements Disposable { /** * Open or create a SQLite3 database * @@ -257,7 +257,20 @@ declare module "bun:sqlite" { * * Internally, this calls `sqlite3_close_v2`. */ - close(): void; + close( + /** + * If `true`, then the database will throw an error if it is in use + * @default false + * + * When true, this calls `sqlite3_close` instead of `sqlite3_close_v2`. + * + * Learn more about this in the [sqlite3 documentation](https://www.sqlite.org/c3ref/close.html). + * + * Bun will automatically call close by default when the database instance is garbage collected. + * In The future, Bun may default `throwOnError` to be true but for backwards compatibility, it is false by default. + */ + throwOnError?: boolean, + ): void; /** * The filename passed when `new Database()` was called @@ -304,6 +317,8 @@ declare module "bun:sqlite" { */ static setCustomSQLite(path: string): boolean; + [Symbol.dispose](): void; + /** * Creates a function that always runs inside a transaction. When the * function is invoked, it will begin a new transaction. When the function @@ -427,6 +442,17 @@ declare module "bun:sqlite" { * ``` */ static deserialize(serialized: NodeJS.TypedArray | ArrayBufferLike, isReadOnly?: boolean): Database; + + /** + * See `sqlite3_file_control` for more information. + * @link https://www.sqlite.org/c3ref/file_control.html + */ + fileControl(op: number, arg?: ArrayBufferView | number): number; + /** + * See `sqlite3_file_control` for more information. + * @link https://www.sqlite.org/c3ref/file_control.html + */ + fileControl(zDbName: string, op: number, arg?: ArrayBufferView | number): number; } /** @@ -455,7 +481,7 @@ declare module "bun:sqlite" { * // => undefined * ``` */ - export class Statement { + export class Statement implements Disposable { /** * Creates a new prepared statement from native code. * @@ -633,6 +659,11 @@ declare module "bun:sqlite" { */ finalize(): void; + /** + * Calls {@link finalize} if it wasn't already called. + */ + [Symbol.dispose](): void; + /** * Return the expanded SQL string for the prepared statement. * @@ -766,6 +797,187 @@ declare module "bun:sqlite" { * @constant 0x04 */ SQLITE_PREPARE_NO_VTAB: number; + + /** + * @constant 1 + */ + SQLITE_FCNTL_LOCKSTATE: number; + /** + * @constant 2 + */ + SQLITE_FCNTL_GET_LOCKPROXYFILE: number; + /** + * @constant 3 + */ + SQLITE_FCNTL_SET_LOCKPROXYFILE: number; + /** + * @constant 4 + */ + SQLITE_FCNTL_LAST_ERRNO: number; + /** + * @constant 5 + */ + SQLITE_FCNTL_SIZE_HINT: number; + /** + * @constant 6 + */ + SQLITE_FCNTL_CHUNK_SIZE: number; + /** + * @constant 7 + */ + SQLITE_FCNTL_FILE_POINTER: number; + /** + * @constant 8 + */ + SQLITE_FCNTL_SYNC_OMITTED: number; + /** + * @constant 9 + */ + SQLITE_FCNTL_WIN32_AV_RETRY: number; + /** + * @constant 10 + * + * Control whether or not the WAL is persisted + * Some versions of macOS configure WAL to be persistent by default. + * + * You can change this with code like the below: + * ```ts + * import { Database } from "bun:sqlite"; + * + * const db = Database.open("mydb.sqlite"); + * db.fileControl(constants.SQLITE_FCNTL_PERSIST_WAL, 0); + * // enable WAL + * db.exec("PRAGMA journal_mode = WAL"); + * // .. do some work + * db.close(); + * ``` + * + */ + SQLITE_FCNTL_PERSIST_WAL: number; + /** + * @constant 11 + */ + SQLITE_FCNTL_OVERWRITE: number; + /** + * @constant 12 + */ + SQLITE_FCNTL_VFSNAME: number; + /** + * @constant 13 + */ + SQLITE_FCNTL_POWERSAFE_OVERWRITE: number; + /** + * @constant 14 + */ + SQLITE_FCNTL_PRAGMA: number; + /** + * @constant 15 + */ + SQLITE_FCNTL_BUSYHANDLER: number; + /** + * @constant 16 + */ + SQLITE_FCNTL_TEMPFILENAME: number; + /** + * @constant 18 + */ + SQLITE_FCNTL_MMAP_SIZE: number; + /** + * @constant 19 + */ + SQLITE_FCNTL_TRACE: number; + /** + * @constant 20 + */ + SQLITE_FCNTL_HAS_MOVED: number; + /** + * @constant 21 + */ + SQLITE_FCNTL_SYNC: number; + /** + * @constant 22 + */ + SQLITE_FCNTL_COMMIT_PHASETWO: number; + /** + * @constant 23 + */ + SQLITE_FCNTL_WIN32_SET_HANDLE: number; + /** + * @constant 24 + */ + SQLITE_FCNTL_WAL_BLOCK: number; + /** + * @constant 25 + */ + SQLITE_FCNTL_ZIPVFS: number; + /** + * @constant 26 + */ + SQLITE_FCNTL_RBU: number; + /** + * @constant 27 + */ + SQLITE_FCNTL_VFS_POINTER: number; + /** + * @constant 28 + */ + SQLITE_FCNTL_JOURNAL_POINTER: number; + /** + * @constant 29 + */ + SQLITE_FCNTL_WIN32_GET_HANDLE: number; + /** + * @constant 30 + */ + SQLITE_FCNTL_PDB: number; + /** + * @constant 31 + */ + SQLITE_FCNTL_BEGIN_ATOMIC_WRITE: number; + /** + * @constant 32 + */ + SQLITE_FCNTL_COMMIT_ATOMIC_WRITE: number; + /** + * @constant 33 + */ + SQLITE_FCNTL_ROLLBACK_ATOMIC_WRITE: number; + /** + * @constant 34 + */ + SQLITE_FCNTL_LOCK_TIMEOUT: number; + /** + * @constant 35 + */ + SQLITE_FCNTL_DATA_VERSION: number; + /** + * @constant 36 + */ + SQLITE_FCNTL_SIZE_LIMIT: number; + /** + * @constant 37 + */ + SQLITE_FCNTL_CKPT_DONE: number; + /** + * @constant 38 + */ + SQLITE_FCNTL_RESERVE_BYTES: number; + /** + * @constant 39 + */ + SQLITE_FCNTL_CKPT_START: number; + /** + * @constant 40 + */ + SQLITE_FCNTL_EXTERNAL_READER: number; + /** + * @constant 41 + */ + SQLITE_FCNTL_CKSM_FILE: number; + /** + * @constant 42 + */ + SQLITE_FCNTL_RESET_CACHE: number; }; /** diff --git a/src/bun.js/bindings/sqlite/JSSQLStatement.cpp b/src/bun.js/bindings/sqlite/JSSQLStatement.cpp index c0b17460235c2..4fb9f9d950bac 100644 --- a/src/bun.js/bindings/sqlite/JSSQLStatement.cpp +++ b/src/bun.js/bindings/sqlite/JSSQLStatement.cpp @@ -1,5 +1,10 @@ + #include "root.h" +#include "JavaScriptCore/ExceptionScope.h" +#include "JavaScriptCore/JSArrayBufferView.h" +#include "JavaScriptCore/JSType.h" + #include "JSSQLStatement.h" #include #include @@ -200,7 +205,7 @@ extern "C" void Bun__closeAllSQLiteDatabasesForTermination() for (auto& db : dbs) { if (db->db) - sqlite3_close_v2(db->db); + sqlite3_close(db->db); } } @@ -335,6 +340,47 @@ class JSSQLStatement : public JSC::JSDestructibleObject { void finishCreation(JSC::VM& vm); }; +static JSValue toJS(JSC::VM& vm, JSC::JSGlobalObject* globalObject, sqlite3_stmt* stmt, int i) +{ + switch (sqlite3_column_type(stmt, i)) { + case SQLITE_INTEGER: { + // https://github.com/oven-sh/bun/issues/1536 + return jsNumberFromSQLite(stmt, i); + } + case SQLITE_FLOAT: { + return jsDoubleNumber(sqlite3_column_double(stmt, i)); + } + // > Note that the SQLITE_TEXT constant was also used in SQLite version + // > 2 for a completely different meaning. Software that links against + // > both SQLite version 2 and SQLite version 3 should use SQLITE3_TEXT, + // > not SQLITE_TEXT. + case SQLITE3_TEXT: { + size_t len = sqlite3_column_bytes(stmt, i); + const unsigned char* text = len > 0 ? sqlite3_column_text(stmt, i) : nullptr; + if (UNLIKELY(text == nullptr || len == 0)) { + return jsEmptyString(vm); + } + + return len < 64 ? jsString(vm, WTF::String::fromUTF8({ text, len })) : JSC::JSValue::decode(Bun__encoding__toStringUTF8(text, len, globalObject)); + } + case SQLITE_BLOB: { + size_t len = sqlite3_column_bytes(stmt, i); + const void* blob = len > 0 ? sqlite3_column_blob(stmt, i) : nullptr; + if (LIKELY(len > 0 && blob != nullptr)) { + JSC::JSUint8Array* array = JSC::JSUint8Array::createUninitialized(globalObject, globalObject->m_typedArrayUint8.get(globalObject), len); + memcpy(array->vector(), blob, len); + return array; + } + + return JSC::JSUint8Array::create(globalObject, globalObject->m_typedArrayUint8.get(globalObject), 0); + } + default: { + break; + } + } + + return jsNull(); +} extern "C" { static JSC_DECLARE_JIT_OPERATION_WITHOUT_WTF_INTERNAL(jsSQLStatementExecuteStatementFunctionGetWithoutTypeChecking, JSC::EncodedJSValue, (JSC::JSGlobalObject * lexicalGlobalObject, JSSQLStatement* castedThis)); } @@ -435,7 +481,7 @@ static void initializeColumnNames(JSC::JSGlobalObject* lexicalGlobalObject, JSSQ // see https://github.com/oven-sh/bun/issues/987 // also see https://github.com/oven-sh/bun/issues/1646 auto& globalObject = *lexicalGlobalObject; - PropertyOffset offset; + auto columnNames = castedThis->columnNames.get(); bool anyHoles = false; for (int i = 0; i < count; i++) { @@ -456,6 +502,7 @@ static void initializeColumnNames(JSC::JSGlobalObject* lexicalGlobalObject, JSSQ } if (LIKELY(!anyHoles)) { + PropertyOffset offset; Structure* structure = globalObject.structureCache().emptyObjectStructureForPrototype(&globalObject, globalObject.objectPrototype(), columnNames->size()); vm.writeBarrier(castedThis, structure); @@ -1226,6 +1273,8 @@ JSC_DEFINE_HOST_FUNCTION(jsSQLStatementOpenStatementFunction, (JSC::JSGlobalObje openFlags = flags.toInt32(lexicalGlobalObject); } + JSValue finalizationTarget = callFrame->argument(2); + sqlite3* db = nullptr; int statusCode = sqlite3_open_v2(path.utf8().data(), &db, openFlags, nullptr); @@ -1244,10 +1293,20 @@ JSC_DEFINE_HOST_FUNCTION(jsSQLStatementOpenStatementFunction, (JSC::JSGlobalObje if (status != SQLITE_OK) { // TODO: log a warning here that defensive mode is unsupported. } - auto count = databases().size(); + auto index = databases().size(); sqlite3_extended_result_codes(db, 1); databases().append(new VersionSqlite3(db)); - RELEASE_AND_RETURN(scope, JSValue::encode(jsNumber(count))); + if (finalizationTarget.isObject()) { + vm.heap.addFinalizer(finalizationTarget.getObject(), [index](JSC::JSCell* ptr) -> void { + auto* db = databases()[index]; + if (!db->db) { + return; + } + sqlite3_close_v2(db->db); + databases()[index]->db = nullptr; + }); + } + RELEASE_AND_RETURN(scope, JSValue::encode(jsNumber(index))); } JSC_DEFINE_HOST_FUNCTION(jsSQLStatementCloseStatementFunction, (JSC::JSGlobalObject * lexicalGlobalObject, JSC::CallFrame* callFrame)) @@ -1270,6 +1329,7 @@ JSC_DEFINE_HOST_FUNCTION(jsSQLStatementCloseStatementFunction, (JSC::JSGlobalObj } JSValue dbNumber = callFrame->argument(0); + JSValue throwOnError = callFrame->argument(1); if (!dbNumber.isNumber()) { throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Expected number"_s)); return JSValue::encode(jsUndefined()); @@ -1282,13 +1342,17 @@ JSC_DEFINE_HOST_FUNCTION(jsSQLStatementCloseStatementFunction, (JSC::JSGlobalObj return JSValue::encode(jsUndefined()); } + bool shouldThrowOnError = (throwOnError.isEmpty() || throwOnError.isUndefined()) ? false : throwOnError.toBoolean(lexicalGlobalObject); + RETURN_IF_EXCEPTION(scope, {}); + sqlite3* db = databases()[dbIndex]->db; // no-op if already closed if (!db) { return JSValue::encode(jsUndefined()); } - int statusCode = sqlite3_close_v2(db); + // sqlite3_close_v2 is used for automatic GC cleanup + int statusCode = shouldThrowOnError ? sqlite3_close(db) : sqlite3_close_v2(db); if (statusCode != SQLITE_OK) { throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, WTF::String::fromUTF8(sqlite3_errstr(statusCode)))); return JSValue::encode(jsUndefined()); @@ -1298,6 +1362,91 @@ JSC_DEFINE_HOST_FUNCTION(jsSQLStatementCloseStatementFunction, (JSC::JSGlobalObj return JSValue::encode(jsUndefined()); } +JSC_DEFINE_HOST_FUNCTION(jsSQLStatementFcntlFunction, (JSC::JSGlobalObject * lexicalGlobalObject, JSC::CallFrame* callFrame)) +{ + JSC::VM& vm = lexicalGlobalObject->vm(); + auto scope = DECLARE_THROW_SCOPE(vm); + + JSValue thisValue = callFrame->thisValue(); + JSSQLStatementConstructor* thisObject = jsDynamicCast(thisValue.getObject()); + if (UNLIKELY(!thisObject)) { + throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Expected SQLStatement"_s)); + return JSValue::encode(jsUndefined()); + } + + if (callFrame->argumentCount() < 2) { + throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Expected 2 arguments"_s)); + return JSValue::encode(jsUndefined()); + } + + JSValue dbNumber = callFrame->argument(0); + JSValue databaseFileName = callFrame->argument(1); + JSValue opNumber = callFrame->argument(2); + JSValue resultValue = callFrame->argument(3); + + if (!dbNumber.isNumber() || !opNumber.isNumber()) { + throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Expected number"_s)); + return JSValue::encode(jsUndefined()); + } + + int dbIndex = dbNumber.toInt32(lexicalGlobalObject); + int op = opNumber.toInt32(lexicalGlobalObject); + + if (dbIndex < 0 || dbIndex >= databases().size()) { + throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Invalid database handle"_s)); + return JSValue::encode(jsUndefined()); + } + + sqlite3* db = databases()[dbIndex]->db; + // no-op if already closed + if (!db) { + return JSValue::encode(jsUndefined()); + } + + CString fileNameStr; + + if (databaseFileName.isString()) { + fileNameStr = databaseFileName.toWTFString(lexicalGlobalObject).utf8(); + RETURN_IF_EXCEPTION(scope, {}); + } + + int resultInt = -1; + void* resultPtr = nullptr; + if (resultValue.isObject()) { + if (auto* view = jsDynamicCast(resultValue.getObject())) { + if (view->isDetached()) { + throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "TypedArray is detached"_s)); + return JSValue::encode(jsUndefined()); + } + + resultPtr = view->vector(); + if (resultPtr == nullptr) { + throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Expected buffer"_s)); + return JSValue::encode(jsUndefined()); + } + } + } else if (resultValue.isNumber()) { + resultInt = resultValue.toInt32(lexicalGlobalObject); + RETURN_IF_EXCEPTION(scope, {}); + + resultPtr = &resultInt; + } else if (resultValue.isNull()) { + + } else { + throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Expected result to be a number, null or a TypedArray"_s)); + return {}; + } + + int statusCode = sqlite3_file_control(db, fileNameStr.isNull() ? nullptr : fileNameStr.data(), op, resultPtr); + + if (statusCode == SQLITE_ERROR) { + throwException(lexicalGlobalObject, scope, createSQLiteError(lexicalGlobalObject, db)); + return JSValue::encode(jsUndefined()); + } + + return JSValue::encode(jsNumber(statusCode)); +} + /* Hash table for constructor */ static const HashTableValue JSSQLStatementConstructorTableValues[] = { { "open"_s, static_cast(JSC::PropertyAttribute::Function), NoIntrinsic, { HashTableValue::NativeFunctionType, jsSQLStatementOpenStatementFunction, 2 } }, @@ -1309,6 +1458,7 @@ static const HashTableValue JSSQLStatementConstructorTableValues[] = { { "setCustomSQLite"_s, static_cast(JSC::PropertyAttribute::Function), NoIntrinsic, { HashTableValue::NativeFunctionType, jsSQLStatementSetCustomSQLite, 1 } }, { "serialize"_s, static_cast(JSC::PropertyAttribute::Function), NoIntrinsic, { HashTableValue::NativeFunctionType, jsSQLStatementSerialize, 1 } }, { "deserialize"_s, static_cast(JSC::PropertyAttribute::Function), NoIntrinsic, { HashTableValue::NativeFunctionType, jsSQLStatementDeserialize, 2 } }, + { "fcntl"_s, static_cast(JSC::PropertyAttribute::Function), NoIntrinsic, { HashTableValue::NativeFunctionType, jsSQLStatementFcntlFunction, 2 } }, }; const ClassInfo JSSQLStatementConstructor::s_info = { "SQLStatement"_s, &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSSQLStatementConstructor) }; @@ -1346,53 +1496,7 @@ static inline JSC::JSValue constructResultObject(JSC::JSGlobalObject* lexicalGlo result = JSC::constructEmptyObject(vm, structure); for (unsigned int i = 0; i < count; i++) { - JSValue value; - - // Loop 1. Fill the rowBuffer with values from SQLite - switch (sqlite3_column_type(stmt, i)) { - case SQLITE_INTEGER: { - // https://github.com/oven-sh/bun/issues/1536 - value = jsNumberFromSQLite(stmt, i); - break; - } - case SQLITE_FLOAT: { - value = jsNumber(sqlite3_column_double(stmt, i)); - break; - } - // > Note that the SQLITE_TEXT constant was also used in SQLite version - // > 2 for a completely different meaning. Software that links against - // > both SQLite version 2 and SQLite version 3 should use SQLITE3_TEXT, - // > not SQLITE_TEXT. - case SQLITE3_TEXT: { - size_t len = sqlite3_column_bytes(stmt, i); - const unsigned char* text = len > 0 ? sqlite3_column_text(stmt, i) : nullptr; - - if (len > 64) { - value = JSC::JSValue::decode(Bun__encoding__toStringUTF8(text, len, lexicalGlobalObject)); - break; - } else { - value = jsString(vm, WTF::String::fromUTF8({ text, len })); - break; - } - } - case SQLITE_BLOB: { - size_t len = sqlite3_column_bytes(stmt, i); - const void* blob = len > 0 ? sqlite3_column_blob(stmt, i) : nullptr; - JSC::JSUint8Array* array = JSC::JSUint8Array::createUninitialized(lexicalGlobalObject, lexicalGlobalObject->m_typedArrayUint8.get(lexicalGlobalObject), len); - - if (LIKELY(blob && len)) - memcpy(array->vector(), blob, len); - - value = array; - break; - } - default: { - value = jsNull(); - break; - } - } - - result->putDirectOffset(vm, i, value); + result->putDirectOffset(vm, i, toJS(vm, lexicalGlobalObject, stmt, i)); } } else { @@ -1403,50 +1507,8 @@ static inline JSC::JSValue constructResultObject(JSC::JSGlobalObject* lexicalGlo } for (int i = 0; i < count; i++) { - auto name = columnNames[i]; - - switch (sqlite3_column_type(stmt, i)) { - case SQLITE_INTEGER: { - // https://github.com/oven-sh/bun/issues/1536 - result->putDirect(vm, name, jsNumberFromSQLite(stmt, i), 0); - break; - } - case SQLITE_FLOAT: { - result->putDirect(vm, name, jsDoubleNumber(sqlite3_column_double(stmt, i)), 0); - break; - } - // > Note that the SQLITE_TEXT constant was also used in SQLite version - // > 2 for a completely different meaning. Software that links against - // > both SQLite version 2 and SQLite version 3 should use SQLITE3_TEXT, - // > not SQLITE_TEXT. - case SQLITE3_TEXT: { - size_t len = sqlite3_column_bytes(stmt, i); - const unsigned char* text = len > 0 ? sqlite3_column_text(stmt, i) : nullptr; - - if (len > 64) { - result->putDirect(vm, name, JSC::JSValue::decode(Bun__encoding__toStringUTF8(text, len, lexicalGlobalObject)), 0); - continue; - } - - result->putDirect(vm, name, jsString(vm, WTF::String::fromUTF8({ text, len })), 0); - break; - } - case SQLITE_BLOB: { - size_t len = sqlite3_column_bytes(stmt, i); - const void* blob = len > 0 ? sqlite3_column_blob(stmt, i) : nullptr; - JSC::JSUint8Array* array = JSC::JSUint8Array::createUninitialized(lexicalGlobalObject, lexicalGlobalObject->m_typedArrayUint8.get(lexicalGlobalObject), len); - - if (LIKELY(blob && len)) - memcpy(array->vector(), blob, len); - - result->putDirect(vm, name, array, 0); - break; - } - default: { - result->putDirect(vm, name, jsNull(), 0); - break; - } - } + const auto& name = columnNames[i]; + result->putDirect(vm, name, toJS(vm, lexicalGlobalObject, stmt, i), 0); } } @@ -1457,53 +1519,15 @@ static inline JSC::JSArray* constructResultRow(JSC::JSGlobalObject* lexicalGloba { int count = castedThis->columnNames->size(); auto& vm = lexicalGlobalObject->vm(); + auto throwScope = DECLARE_THROW_SCOPE(vm); JSC::JSArray* result = JSArray::create(vm, lexicalGlobalObject->arrayStructureForIndexingTypeDuringAllocation(ArrayWithContiguous), count); auto* stmt = castedThis->stmt; for (int i = 0; i < count; i++) { - - switch (sqlite3_column_type(stmt, i)) { - case SQLITE_INTEGER: { - // https://github.com/oven-sh/bun/issues/1536 - result->putDirectIndex(lexicalGlobalObject, i, jsNumberFromSQLite(stmt, i)); - break; - } - case SQLITE_FLOAT: { - result->putDirectIndex(lexicalGlobalObject, i, jsDoubleNumber(sqlite3_column_double(stmt, i))); - break; - } - // > Note that the SQLITE_TEXT constant was also used in SQLite version - // > 2 for a completely different meaning. Software that links against - // > both SQLite version 2 and SQLite version 3 should use SQLITE3_TEXT, - // > not SQLITE_TEXT. - case SQLITE_TEXT: { - size_t len = sqlite3_column_bytes(stmt, i); - const unsigned char* text = len > 0 ? sqlite3_column_text(stmt, i) : nullptr; - if (UNLIKELY(text == nullptr || len == 0)) { - result->putDirectIndex(lexicalGlobalObject, i, jsEmptyString(vm)); - continue; - } - result->putDirectIndex(lexicalGlobalObject, i, len < 64 ? jsString(vm, WTF::String::fromUTF8({ text, len })) : JSC::JSValue::decode(Bun__encoding__toStringUTF8(text, len, lexicalGlobalObject))); - break; - } - case SQLITE_BLOB: { - size_t len = sqlite3_column_bytes(stmt, i); - const void* blob = len > 0 ? sqlite3_column_blob(stmt, i) : nullptr; - if (LIKELY(len > 0 && blob != nullptr)) { - JSC::JSUint8Array* array = JSC::JSUint8Array::createUninitialized(lexicalGlobalObject, lexicalGlobalObject->m_typedArrayUint8.get(lexicalGlobalObject), len); - memcpy(array->vector(), blob, len); - result->putDirectIndex(lexicalGlobalObject, i, array); - } else { - result->putDirectIndex(lexicalGlobalObject, i, JSC::JSUint8Array::create(lexicalGlobalObject, lexicalGlobalObject->m_typedArrayUint8.get(lexicalGlobalObject), 0)); - } - break; - } - default: { - result->putDirectIndex(lexicalGlobalObject, i, jsNull()); - break; - } - } + JSValue value = toJS(vm, lexicalGlobalObject, stmt, i); + RETURN_IF_EXCEPTION(throwScope, nullptr); + result->putDirectIndex(lexicalGlobalObject, i, value); } return result; diff --git a/src/bun.js/bindings/sqlite/lazy_sqlite3.h b/src/bun.js/bindings/sqlite/lazy_sqlite3.h index 06c0a83e5551f..00a1c292d07d7 100644 --- a/src/bun.js/bindings/sqlite/lazy_sqlite3.h +++ b/src/bun.js/bindings/sqlite/lazy_sqlite3.h @@ -20,6 +20,8 @@ typedef int (*lazy_sqlite3_bind_parameter_index_type)(sqlite3_stmt*, const char* typedef int (*lazy_sqlite3_changes_type)(sqlite3*); typedef int (*lazy_sqlite3_clear_bindings_type)(sqlite3_stmt*); typedef int (*lazy_sqlite3_close_v2_type)(sqlite3*); +typedef int (*lazy_sqlite3_close_type)(sqlite3*); +typedef int (*lazy_sqlite3_file_control_type)(sqlite3*, const char* zDbName, int op, void* pArg); typedef int (*lazy_sqlite3_extended_result_codes_type)(sqlite3*, int onoff); typedef const void* (*lazy_sqlite3_column_blob_type)(sqlite3_stmt*, int iCol); typedef double (*lazy_sqlite3_column_double_type)(sqlite3_stmt*, int iCol); @@ -100,6 +102,8 @@ static lazy_sqlite3_bind_text16_type lazy_sqlite3_bind_text16; static lazy_sqlite3_changes_type lazy_sqlite3_changes; static lazy_sqlite3_clear_bindings_type lazy_sqlite3_clear_bindings; static lazy_sqlite3_close_v2_type lazy_sqlite3_close_v2; +static lazy_sqlite3_close_type lazy_sqlite3_close; +static lazy_sqlite3_file_control_type lazy_sqlite3_file_control; static lazy_sqlite3_column_blob_type lazy_sqlite3_column_blob; static lazy_sqlite3_column_bytes_type lazy_sqlite3_column_bytes; static lazy_sqlite3_column_bytes16_type lazy_sqlite3_column_bytes16; @@ -147,6 +151,8 @@ static lazy_sqlite3_memory_used_type lazy_sqlite3_memory_used; #define sqlite3_changes lazy_sqlite3_changes #define sqlite3_clear_bindings lazy_sqlite3_clear_bindings #define sqlite3_close_v2 lazy_sqlite3_close_v2 +#define sqlite3_close lazy_sqlite3_close +#define sqlite3_file_control lazy_sqlite3_file_control #define sqlite3_column_blob lazy_sqlite3_column_blob #define sqlite3_column_bytes lazy_sqlite3_column_bytes #define sqlite3_column_count lazy_sqlite3_column_count @@ -226,6 +232,8 @@ static int lazyLoadSQLite() lazy_sqlite3_changes = (lazy_sqlite3_changes_type)dlsym(sqlite3_handle, "sqlite3_changes"); lazy_sqlite3_clear_bindings = (lazy_sqlite3_clear_bindings_type)dlsym(sqlite3_handle, "sqlite3_clear_bindings"); lazy_sqlite3_close_v2 = (lazy_sqlite3_close_v2_type)dlsym(sqlite3_handle, "sqlite3_close_v2"); + lazy_sqlite3_close = (lazy_sqlite3_close_type)dlsym(sqlite3_handle, "sqlite3_close"); + lazy_sqlite3_file_control = (lazy_sqlite3_file_control_type)dlsym(sqlite3_handle, "sqlite3_file_control"); lazy_sqlite3_column_blob = (lazy_sqlite3_column_blob_type)dlsym(sqlite3_handle, "sqlite3_column_blob"); lazy_sqlite3_column_bytes = (lazy_sqlite3_column_bytes_type)dlsym(sqlite3_handle, "sqlite3_column_bytes"); lazy_sqlite3_column_count = (lazy_sqlite3_column_count_type)dlsym(sqlite3_handle, "sqlite3_column_count"); diff --git a/src/js/bun/sqlite.ts b/src/js/bun/sqlite.ts index a01b55c6b8164..6744c569fea41 100644 --- a/src/js/bun/sqlite.ts +++ b/src/js/bun/sqlite.ts @@ -31,6 +31,48 @@ const constants = { SQLITE_PREPARE_PERSISTENT: 0x01, SQLITE_PREPARE_NORMALIZE: 0x02, SQLITE_PREPARE_NO_VTAB: 0x04, + + SQLITE_FCNTL_LOCKSTATE: 1, + SQLITE_FCNTL_GET_LOCKPROXYFILE: 2, + SQLITE_FCNTL_SET_LOCKPROXYFILE: 3, + SQLITE_FCNTL_LAST_ERRNO: 4, + SQLITE_FCNTL_SIZE_HINT: 5, + SQLITE_FCNTL_CHUNK_SIZE: 6, + SQLITE_FCNTL_FILE_POINTER: 7, + SQLITE_FCNTL_SYNC_OMITTED: 8, + SQLITE_FCNTL_WIN32_AV_RETRY: 9, + SQLITE_FCNTL_PERSIST_WAL: 10, + SQLITE_FCNTL_OVERWRITE: 11, + SQLITE_FCNTL_VFSNAME: 12, + SQLITE_FCNTL_POWERSAFE_OVERWRITE: 13, + SQLITE_FCNTL_PRAGMA: 14, + SQLITE_FCNTL_BUSYHANDLER: 15, + SQLITE_FCNTL_TEMPFILENAME: 16, + SQLITE_FCNTL_MMAP_SIZE: 18, + SQLITE_FCNTL_TRACE: 19, + SQLITE_FCNTL_HAS_MOVED: 20, + SQLITE_FCNTL_SYNC: 21, + SQLITE_FCNTL_COMMIT_PHASETWO: 22, + SQLITE_FCNTL_WIN32_SET_HANDLE: 23, + SQLITE_FCNTL_WAL_BLOCK: 24, + SQLITE_FCNTL_ZIPVFS: 25, + SQLITE_FCNTL_RBU: 26, + SQLITE_FCNTL_VFS_POINTER: 27, + SQLITE_FCNTL_JOURNAL_POINTER: 28, + SQLITE_FCNTL_WIN32_GET_HANDLE: 29, + SQLITE_FCNTL_PDB: 30, + SQLITE_FCNTL_BEGIN_ATOMIC_WRITE: 31, + SQLITE_FCNTL_COMMIT_ATOMIC_WRITE: 32, + SQLITE_FCNTL_ROLLBACK_ATOMIC_WRITE: 33, + SQLITE_FCNTL_LOCK_TIMEOUT: 34, + SQLITE_FCNTL_DATA_VERSION: 35, + SQLITE_FCNTL_SIZE_LIMIT: 36, + SQLITE_FCNTL_CKPT_DONE: 37, + SQLITE_FCNTL_RESERVE_BYTES: 38, + SQLITE_FCNTL_CKPT_START: 39, + SQLITE_FCNTL_EXTERNAL_READER: 40, + SQLITE_FCNTL_CKSM_FILE: 41, + SQLITE_FCNTL_RESET_CACHE: 42, }; var SQL; @@ -161,6 +203,12 @@ class Statement { this.isFinalized = true; return this.#raw.finalize(...args); } + + [Symbol.dispose]() { + if (!this.isFinalized) { + this.finalize(); + } + } } var cachedCount = Symbol.for("Bun.Database.cache.count"); @@ -213,7 +261,7 @@ class Database { SQL = $cpp("JSSQLStatement.cpp", "createJSSQLStatementConstructor"); } - this.#handle = SQL.open(anonymous ? ":memory:" : filename, flags); + this.#handle = SQL.open(anonymous ? ":memory:" : filename, flags, this); this.filename = filename; } @@ -222,7 +270,7 @@ class Database { #cachedQueriesLengths = []; #cachedQueriesValues = []; filename; - + #hasClosed = false; get handle() { return this.#handle; } @@ -255,6 +303,12 @@ class Database { return new Database(serialized, isReadOnly ? constants.SQLITE_OPEN_READONLY : 0); } + [Symbol.dispose]() { + if (!this.#hasClosed) { + this.close(true); + } + } + static setCustomSQLite(path) { if (!SQL) { SQL = $cpp("JSSQLStatement.cpp", "createJSSQLStatementConstructor"); @@ -263,13 +317,24 @@ class Database { return SQL.setCustomSQLite(path); } - close() { + fileControl(cmd, arg) { + const handle = this.#handle; + + if (arguments.length <= 2) { + return SQL.fcntl(handle, null, arguments[0], arguments[1]); + } + + return SQL.fcntl(handle, ...arguments); + } + + close(throwOnError = false) { this.clearQueryCache(); - return SQL.close(this.#handle); + this.#hasClosed = true; + return SQL.close(this.#handle, throwOnError); } clearQueryCache() { for (let item of this.#cachedQueriesValues) { - item.finalize(); + item?.finalize?.(); } this.#cachedQueriesKeys.length = 0; this.#cachedQueriesValues.length = 0; diff --git a/test/js/bun/sqlite/sqlite.test.js b/test/js/bun/sqlite/sqlite.test.js index 4578acb730e8a..f75c2f03153c6 100644 --- a/test/js/bun/sqlite/sqlite.test.js +++ b/test/js/bun/sqlite/sqlite.test.js @@ -1,8 +1,8 @@ import { expect, it, describe } from "bun:test"; import { Database, constants, SQLiteError } from "bun:sqlite"; -import { existsSync, fstat, realpathSync, rmSync, writeFileSync } from "fs"; +import { existsSync, fstat, readdirSync, realpathSync, rmSync, writeFileSync } from "fs"; import { spawnSync } from "bun"; -import { bunExe } from "harness"; +import { bunExe, isWindows, tempDirWithFiles } from "harness"; import { tmpdir } from "os"; import path from "path"; @@ -777,3 +777,107 @@ it.skipIf( expect(db.prepare("SELECT SQRT(0.25)").all()).toEqual([{ "SQRT(0.25)": 0.5 }]); expect(db.prepare("SELECT TAN(0.25)").all()).toEqual([{ "TAN(0.25)": 0.25534192122103627 }]); }); + +it("should close with WAL enabled", () => { + const dir = tempDirWithFiles("sqlite-wal-test", { "empty.txt": "" }); + const file = path.join(dir, "my.db"); + const db = new Database(file); + db.exec("PRAGMA journal_mode = WAL"); + db.fileControl(constants.SQLITE_FCNTL_PERSIST_WAL, 0); + db.exec("CREATE TABLE foo (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT)"); + db.exec("INSERT INTO foo (name) VALUES ('foo')"); + expect(db.query("SELECT * FROM foo").all()).toEqual([{ id: 1, name: "foo" }]); + db.exec("PRAGMA wal_checkpoint(truncate)"); + db.close(); + expect(readdirSync(dir).sort()).toEqual(["empty.txt", "my.db"]); +}); + +it("close(true) should throw an error if the database is in use", () => { + const db = new Database(":memory:"); + db.exec("CREATE TABLE foo (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT)"); + db.exec("INSERT INTO foo (name) VALUES ('foo')"); + const prepared = db.prepare("SELECT * FROM foo"); + expect(() => db.close(true)).toThrow("database is locked"); + prepared.finalize(); + expect(() => db.close(true)).not.toThrow(); +}); + +it("close() should NOT throw an error if the database is in use", () => { + const db = new Database(":memory:"); + db.exec("CREATE TABLE foo (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT)"); + db.exec("INSERT INTO foo (name) VALUES ('foo')"); + const prepared = db.prepare("SELECT * FROM foo"); + expect(() => db.close()).not.toThrow("database is locked"); +}); + +it("should dispose AND throw an error if the database is in use", () => { + expect(() => { + { + using db = new Database(":memory:"); + db.exec("CREATE TABLE foo (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT)"); + db.exec("INSERT INTO foo (name) VALUES ('foo')"); + var prepared = db.prepare("SELECT * FROM foo"); + } + }).toThrow("database is locked"); +}); + +it("should dispose", () => { + expect(() => { + { + using db = new Database(":memory:"); + db.exec("CREATE TABLE foo (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT)"); + db.exec("INSERT INTO foo (name) VALUES ('foo')"); + } + }).not.toThrow(); +}); + +it("can continue to use existing statements after database has been GC'd", async () => { + var called = false; + var registry = new FinalizationRegistry(() => { + called = true; + }); + function leakTheStatement() { + const db = new Database(":memory:"); + db.exec("CREATE TABLE foo (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT)"); + db.exec("INSERT INTO foo (name) VALUES ('foo')"); + const prepared = db.prepare("SELECT * FROM foo"); + registry.register(db); + return prepared; + } + + const stmt = leakTheStatement(); + Bun.gc(true); + await Bun.sleep(1); + Bun.gc(true); + expect(stmt.all()).toEqual([{ id: 1, name: "foo" }]); + stmt.finalize(); + expect(() => stmt.all()).toThrow(); + if (!isWindows) { + // on Windows, FinalizationRegistry is more flaky than on POSIX. + expect(called).toBe(true); + } +}); + +it("statements should be disposable", () => { + { + using db = new Database("mydb.sqlite"); + using query = db.query("select 'Hello world' as message;"); + console.log(query.get()); // => { message: "Hello world" } + } +}); + +it("query should work if the cached statement was finalized", () => { + { + using db = new Database("mydb.sqlite"); + { + using query = db.query("select 'Hello world' as message;"); + var prevQuery = query; + query.get(); + } + { + using query = db.query("select 'Hello world' as message;"); + expect(() => query.get()).not.toThrow(); + } + expect(() => prevQuery.get()).toThrow(); + } +}); From 24a411f9044536f8d4d8c64e17ddf2ee8bd031ed Mon Sep 17 00:00:00 2001 From: Georgijs <48869301+gvilums@users.noreply.github.com> Date: Mon, 15 Apr 2024 14:02:28 -0700 Subject: [PATCH 2/8] Correctly handle duplicate column names in sqlite joins (#10285) * add tests * working * cleanup * fix compile * fix naming and comment * fix lints in test * apply suggested fixes --- packages/bun-usockets/src/crypto/openssl.c | 2 - src/bun.js/bindings/sqlite/JSSQLStatement.cpp | 109 ++++++++++++++++-- test/js/bun/sqlite/sqlite.test.js | 81 ++++++++++++- 3 files changed, 176 insertions(+), 16 deletions(-) diff --git a/packages/bun-usockets/src/crypto/openssl.c b/packages/bun-usockets/src/crypto/openssl.c index e5514680b73f5..9f502ecb6044b 100644 --- a/packages/bun-usockets/src/crypto/openssl.c +++ b/packages/bun-usockets/src/crypto/openssl.c @@ -257,8 +257,6 @@ void us_internal_on_ssl_handshake( struct us_internal_ssl_socket_t * us_internal_ssl_socket_close(struct us_internal_ssl_socket_t *s, int code, void *reason) { - struct us_internal_ssl_socket_context_t *context = - (struct us_internal_ssl_socket_context_t *)us_socket_context(0, &s->s); if (s->handshake_state != HANDSHAKE_COMPLETED) { // if we have some pending handshake we cancel it and try to check the diff --git a/src/bun.js/bindings/sqlite/JSSQLStatement.cpp b/src/bun.js/bindings/sqlite/JSSQLStatement.cpp index 4fb9f9d950bac..6c8912fdc7c2c 100644 --- a/src/bun.js/bindings/sqlite/JSSQLStatement.cpp +++ b/src/bun.js/bindings/sqlite/JSSQLStatement.cpp @@ -34,6 +34,8 @@ #include #include "BunBuiltinNames.h" #include "sqlite3_error_codes.h" +#include "wtf/BitVector.h" +#include "wtf/Vector.h" #include /* ******************************************************************************** */ @@ -322,6 +324,9 @@ class JSSQLStatement : public JSC::JSDestructibleObject { VersionSqlite3* version_db; uint64_t version; bool hasExecuted = false; + // Tracks which columns are valid in the current result set. Used to handle duplicate column names. + // The bit at index i is set if the column at index i is valid. + WTF::BitVector validColumns; std::unique_ptr columnNames; mutable JSC::WriteBarrier _prototype; mutable JSC::WriteBarrier _structure; @@ -462,6 +467,7 @@ static void initializeColumnNames(JSC::JSGlobalObject* lexicalGlobalObject, JSSQ castedThis->columnNames->propertyNameMode(), castedThis->columnNames->privateSymbolMode())); } + castedThis->validColumns.clearAll(); castedThis->update_version(); JSC::VM& vm = lexicalGlobalObject->vm(); @@ -484,7 +490,7 @@ static void initializeColumnNames(JSC::JSGlobalObject* lexicalGlobalObject, JSSQ auto columnNames = castedThis->columnNames.get(); bool anyHoles = false; - for (int i = 0; i < count; i++) { + for (int i = count - 1; i >= 0; i--) { const char* name = sqlite3_column_name(stmt, i); if (name == nullptr) { @@ -498,7 +504,18 @@ static void initializeColumnNames(JSC::JSGlobalObject* lexicalGlobalObject, JSSQ break; } - columnNames->add(Identifier::fromString(vm, WTF::String::fromUTF8({ name, len }))); + // When joining multiple tables, the same column names can appear multiple times + // columnNames de-dupes property names internally + // We can't have two properties with the same name, so we use validColumns to track this. + auto preCount = columnNames->size(); + columnNames->add( + Identifier::fromString(vm, WTF::String::fromUTF8({name, len})) + ); + auto curCount = columnNames->size(); + + if (preCount != curCount) { + castedThis->validColumns.set(i); + } } if (LIKELY(!anyHoles)) { @@ -506,6 +523,10 @@ static void initializeColumnNames(JSC::JSGlobalObject* lexicalGlobalObject, JSSQ Structure* structure = globalObject.structureCache().emptyObjectStructureForPrototype(&globalObject, globalObject.objectPrototype(), columnNames->size()); vm.writeBarrier(castedThis, structure); + // We iterated over the columns in reverse order so we need to reverse the columnNames here + // Importantly we reverse before adding the properties to the structure to ensure that index accesses + // later refer to the correct property. + columnNames->data()->propertyNameVector().reverse(); for (const auto& propertyName : *columnNames) { structure = Structure::addPropertyTransition(vm, structure, propertyName, 0, offset); } @@ -520,6 +541,7 @@ static void initializeColumnNames(JSC::JSGlobalObject* lexicalGlobalObject, JSSQ castedThis->columnNames->vm(), castedThis->columnNames->propertyNameMode(), castedThis->columnNames->privateSymbolMode())); + castedThis->validColumns.clearAll(); } } @@ -531,7 +553,7 @@ static void initializeColumnNames(JSC::JSGlobalObject* lexicalGlobalObject, JSSQ // see https://github.com/oven-sh/bun/issues/987 JSC::JSObject* object = JSC::constructEmptyObject(lexicalGlobalObject, lexicalGlobalObject->objectPrototype(), std::min(static_cast(count), JSFinalObject::maxInlineCapacity)); - for (int i = 0; i < count; i++) { + for (int i = count - 1; i >= 0; i--) { const char* name = sqlite3_column_name(stmt, i); if (name == nullptr) @@ -562,9 +584,18 @@ static void initializeColumnNames(JSC::JSGlobalObject* lexicalGlobalObject, JSSQ } } - object->putDirect(vm, key, primitive, 0); + auto preCount = castedThis->columnNames->size(); castedThis->columnNames->add(key); + auto curCount = castedThis->columnNames->size(); + + // only put the property if it's not a duplicate + if (preCount != curCount) { + castedThis->validColumns.set(i); + object->putDirect(vm, key, primitive, 0); + } } + // We iterated over the columns in reverse order so we need to reverse the columnNames here + castedThis->columnNames->data()->propertyNameVector().reverse(); castedThis->_prototype.set(vm, castedThis, object); } @@ -1495,8 +1526,15 @@ static inline JSC::JSValue constructResultObject(JSC::JSGlobalObject* lexicalGlo if (auto* structure = castedThis->_structure.get()) { result = JSC::constructEmptyObject(vm, structure); - for (unsigned int i = 0; i < count; i++) { - result->putDirectOffset(vm, i, toJS(vm, lexicalGlobalObject, stmt, i)); + // i: the index of columns returned from SQLite + // j: the index of object property + for (int i = 0, j = 0; j < count; i++, j++) { + if (!castedThis->validColumns.get(i)) { + // this column is duplicate, skip + j -= 1; + continue; + } + result->putDirectOffset(vm, j, toJS(vm, lexicalGlobalObject, stmt, i)); } } else { @@ -1506,9 +1544,56 @@ static inline JSC::JSValue constructResultObject(JSC::JSGlobalObject* lexicalGlo result = JSC::JSFinalObject::create(vm, JSC::JSFinalObject::createStructure(vm, lexicalGlobalObject, lexicalGlobalObject->objectPrototype(), JSFinalObject::maxInlineCapacity)); } - for (int i = 0; i < count; i++) { - const auto& name = columnNames[i]; + for (int i = 0, j = 0; j < count; i++, j++) { + if (!castedThis->validColumns.get(i)) { + j -= 1; + continue; + } + auto name = columnNames[j]; result->putDirect(vm, name, toJS(vm, lexicalGlobalObject, stmt, i), 0); + + switch (sqlite3_column_type(stmt, i)) { + case SQLITE_INTEGER: { + // https://github.com/oven-sh/bun/issues/1536 + result->putDirect(vm, name, jsNumberFromSQLite(stmt, i), 0); + break; + } + case SQLITE_FLOAT: { + result->putDirect(vm, name, jsDoubleNumber(sqlite3_column_double(stmt, i)), 0); + break; + } + // > Note that the SQLITE_TEXT constant was also used in SQLite version + // > 2 for a completely different meaning. Software that links against + // > both SQLite version 2 and SQLite version 3 should use SQLITE3_TEXT, + // > not SQLITE_TEXT. + case SQLITE3_TEXT: { + size_t len = sqlite3_column_bytes(stmt, i); + const unsigned char* text = len > 0 ? sqlite3_column_text(stmt, i) : nullptr; + + if (len > 64) { + result->putDirect(vm, name, JSC::JSValue::decode(Bun__encoding__toStringUTF8(text, len, lexicalGlobalObject)), 0); + continue; + } + + result->putDirect(vm, name, jsString(vm, WTF::String::fromUTF8({ text, len })), 0); + break; + } + case SQLITE_BLOB: { + size_t len = sqlite3_column_bytes(stmt, i); + const void* blob = len > 0 ? sqlite3_column_blob(stmt, i) : nullptr; + JSC::JSUint8Array* array = JSC::JSUint8Array::createUninitialized(lexicalGlobalObject, lexicalGlobalObject->m_typedArrayUint8.get(lexicalGlobalObject), len); + + if (LIKELY(blob && len)) + memcpy(array->vector(), blob, len); + + result->putDirect(vm, name, array, 0); + break; + } + default: { + result->putDirect(vm, name, jsNull(), 0); + break; + } + } } } @@ -1524,10 +1609,14 @@ static inline JSC::JSArray* constructResultRow(JSC::JSGlobalObject* lexicalGloba JSC::JSArray* result = JSArray::create(vm, lexicalGlobalObject->arrayStructureForIndexingTypeDuringAllocation(ArrayWithContiguous), count); auto* stmt = castedThis->stmt; - for (int i = 0; i < count; i++) { + for (int i = 0, j = 0; j < count; i++, j++) { + if (!castedThis->validColumns.get(i)) { + j -= 1; + continue; + } JSValue value = toJS(vm, lexicalGlobalObject, stmt, i); RETURN_IF_EXCEPTION(throwScope, nullptr); - result->putDirectIndex(lexicalGlobalObject, i, value); + result->putDirectIndex(lexicalGlobalObject, j, value); } return result; diff --git a/test/js/bun/sqlite/sqlite.test.js b/test/js/bun/sqlite/sqlite.test.js index f75c2f03153c6..60a9d01b1cdee 100644 --- a/test/js/bun/sqlite/sqlite.test.js +++ b/test/js/bun/sqlite/sqlite.test.js @@ -778,6 +778,77 @@ it.skipIf( expect(db.prepare("SELECT TAN(0.25)").all()).toEqual([{ "TAN(0.25)": 0.25534192122103627 }]); }); +it("issue#6597", () => { + // better-sqlite3 returns the last value of duplicate fields + const db = new Database(":memory:"); + db.run("CREATE TABLE Users (Id INTEGER PRIMARY KEY, Name VARCHAR(255), CreatedAt TIMESTAMP)"); + db.run( + "CREATE TABLE Cars (Id INTEGER PRIMARY KEY, Driver INTEGER, CreatedAt TIMESTAMP, FOREIGN KEY (Driver) REFERENCES Users(Id))", + ); + db.run('INSERT INTO Users (Id, Name, CreatedAt) VALUES (1, "Alice", "2022-01-01");'); + db.run('INSERT INTO Cars (Id, Driver, CreatedAt) VALUES (2, 1, "2023-01-01");'); + const result = db.prepare("SELECT * FROM Cars JOIN Users ON Driver=Users.Id").get(); + expect(result).toStrictEqual({ + Id: 1, + Driver: 1, + CreatedAt: "2022-01-01", + Name: "Alice", + }); + db.close(); +}); + +it("issue#6597 with many columns", () => { + // better-sqlite3 returns the last value of duplicate fields + const db = new Database(":memory:"); + const count = 100; + const columns = Array.from({ length: count }, (_, i) => `col${i}`); + const values_foo = Array.from({ length: count }, (_, i) => `'foo${i}'`); + const values_bar = Array.from({ length: count }, (_, i) => `'bar${i}'`); + values_bar[0] = values_foo[0]; + db.run(`CREATE TABLE foo (${columns.join(",")})`); + db.run(`CREATE TABLE bar (${columns.join(",")})`); + db.run(`INSERT INTO foo (${columns.join(",")}) VALUES (${values_foo.join(",")})`); + db.run(`INSERT INTO bar (${columns.join(",")}) VALUES (${values_bar.join(",")})`); + const result = db.prepare("SELECT * FROM foo JOIN bar ON foo.col0 = bar.col0").get(); + expect(result.col0).toBe("foo0"); + for (let i = 1; i < count; i++) { + expect(result[`col${i}`]).toBe(`bar${i}`); + } + db.close(); +}); + +it("issue#7147", () => { + const db = new Database(":memory:"); + db.exec("CREATE TABLE foos (foo_id INTEGER NOT NULL PRIMARY KEY, foo_a TEXT, foo_b TEXT)"); + db.exec( + "CREATE TABLE bars (bar_id INTEGER NOT NULL PRIMARY KEY, foo_id INTEGER NOT NULL, bar_a INTEGER, bar_b INTEGER, FOREIGN KEY (foo_id) REFERENCES foos (foo_id))", + ); + db.exec("INSERT INTO foos VALUES (1, 'foo_1', 'foo_2')"); + db.exec("INSERT INTO bars VALUES (1, 1, 'bar_1', 'bar_2')"); + db.exec("INSERT INTO bars VALUES (2, 1, 'baz_3', 'baz_4')"); + const query = db.query("SELECT f.*, b.* FROM foos f JOIN bars b ON b.foo_id = f.foo_id"); + const result = query.all(); + expect(result).toStrictEqual([ + { + foo_id: 1, + foo_a: "foo_1", + foo_b: "foo_2", + bar_id: 1, + bar_a: "bar_1", + bar_b: "bar_2", + }, + { + foo_id: 1, + foo_a: "foo_1", + foo_b: "foo_2", + bar_id: 2, + bar_a: "baz_3", + bar_b: "baz_4", + }, + ]); + db.close(); +}); + it("should close with WAL enabled", () => { const dir = tempDirWithFiles("sqlite-wal-test", { "empty.txt": "" }); const file = path.join(dir, "my.db"); @@ -812,11 +883,12 @@ it("close() should NOT throw an error if the database is in use", () => { it("should dispose AND throw an error if the database is in use", () => { expect(() => { + let prepared; { using db = new Database(":memory:"); db.exec("CREATE TABLE foo (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT)"); db.exec("INSERT INTO foo (name) VALUES ('foo')"); - var prepared = db.prepare("SELECT * FROM foo"); + prepared = db.prepare("SELECT * FROM foo"); } }).toThrow("database is locked"); }); @@ -832,8 +904,8 @@ it("should dispose", () => { }); it("can continue to use existing statements after database has been GC'd", async () => { - var called = false; - var registry = new FinalizationRegistry(() => { + let called = false; + const registry = new FinalizationRegistry(() => { called = true; }); function leakTheStatement() { @@ -868,10 +940,11 @@ it("statements should be disposable", () => { it("query should work if the cached statement was finalized", () => { { + let prevQuery; using db = new Database("mydb.sqlite"); { using query = db.query("select 'Hello world' as message;"); - var prevQuery = query; + prevQuery = query; query.get(); } { From 5a81dc8e33a09e13eb974378eeb3ae2240c1a080 Mon Sep 17 00:00:00 2001 From: Dylan Conway <35280289+dylan-conway@users.noreply.github.com> Date: Mon, 15 Apr 2024 18:29:34 -0700 Subject: [PATCH 3/8] fix(install): fix dependency install order (#10240) * packages wait for parent trees before install * use `directoryExistsAt` * missing increment * fix faccessat * swap destination and target * update * force * only create destination dir before installing package * fix windows symlink/junction * increment, false on extracting * done --- src/bun.zig | 3 +- src/fmt.zig | 1 - src/install/install.zig | 779 ++++++++++++++++++++++--------------- src/install/lockfile.zig | 13 + src/install/resolution.zig | 8 + src/resolver/resolver.zig | 2 +- src/sys.zig | 28 +- 7 files changed, 510 insertions(+), 324 deletions(-) diff --git a/src/bun.zig b/src/bun.zig index d2f6ce5ee9eda..100f524bafd1b 100644 --- a/src/bun.zig +++ b/src/bun.zig @@ -28,6 +28,7 @@ else pub const huge_allocator_threshold: comptime_int = @import("./memory_allocator.zig").huge_threshold; pub const callmod_inline: std.builtin.CallModifier = if (builtin.mode == .Debug) .auto else .always_inline; +pub const callconv_inline: std.builtin.CallingConvention = if (builtin.mode == .Debug) .Unspecified else .Inline; /// We cannot use a threadlocal memory allocator for FileSystem-related things /// FileSystem is a singleton. @@ -3089,7 +3090,7 @@ pub inline fn debugAssert(cheap_value_only_plz: bool) void { } } -pub inline fn assert(value: bool) void { +pub fn assert(value: bool) callconv(callconv_inline) void { if (comptime !Environment.allow_assert) { return; } diff --git a/src/fmt.zig b/src/fmt.zig index 259729247c4f8..8d6dae069fbe2 100644 --- a/src/fmt.zig +++ b/src/fmt.zig @@ -99,7 +99,6 @@ pub inline fn utf16(slice_: []const u16) FormatUTF16 { pub const FormatUTF16 = struct { buf: []const u16, - escape_backslashes: bool = false, path_fmt_opts: ?PathFormatOptions = null, pub fn format(self: @This(), comptime _: []const u8, _: anytype, writer: anytype) !void { if (self.path_fmt_opts) |opts| { diff --git a/src/install/install.zig b/src/install/install.zig index f135cfd57a4bd..90da7aae7bde0 100644 --- a/src/install/install.zig +++ b/src/install/install.zig @@ -874,7 +874,6 @@ pub const ExtractData = struct { pub const PackageInstall = struct { cache_dir: std.fs.Dir, - destination_dir: std.fs.Dir, cache_dir_subpath: stringZ = "", destination_dir_subpath: stringZ = "", destination_dir_subpath_buf: []u8, @@ -886,7 +885,7 @@ pub const PackageInstall = struct { package_name: string, package_version: string, file_count: u32 = 0, - node_modules: *const NodeModulesFolder, + node_modules: *const PackageManager.NodeModulesFolder, const debug = Output.scoped(.install, true); @@ -963,7 +962,12 @@ pub const PackageInstall = struct { // 1. verify that .bun-tag exists (was it installed from bun?) // 2. check .bun-tag against the resolved version - fn verifyGitResolution(this: *PackageInstall, repo: *const Repository, buf: []const u8) bool { + fn verifyGitResolution( + this: *PackageInstall, + repo: *const Repository, + buf: []const u8, + root_node_modules_dir: std.fs.Dir, + ) bool { bun.copy(u8, this.destination_dir_subpath_buf[this.destination_dir_subpath.len..], std.fs.path.sep_str ++ ".bun-tag"); this.destination_dir_subpath_buf[this.destination_dir_subpath.len + std.fs.path.sep_str.len + ".bun-tag".len] = 0; const bun_tag_path: [:0]u8 = this.destination_dir_subpath_buf[0 .. this.destination_dir_subpath.len + std.fs.path.sep_str.len + ".bun-tag".len :0]; @@ -971,8 +975,13 @@ pub const PackageInstall = struct { var git_tag_stack_fallback = std.heap.stackFallback(2048, bun.default_allocator); const allocator = git_tag_stack_fallback.get(); + var destination_dir = this.node_modules.openDir(root_node_modules_dir) catch return false; + defer { + if (std.fs.cwd().fd != destination_dir.fd) destination_dir.close(); + } + const bun_tag_file = File.readFrom( - this.destination_dir, + destination_dir, bun_tag_path, allocator, ).unwrap() catch return false; @@ -985,15 +994,16 @@ pub const PackageInstall = struct { this: *PackageInstall, resolution: *const Resolution, buf: []const u8, + root_node_modules_dir: std.fs.Dir, ) bool { return switch (resolution.tag) { - .git => this.verifyGitResolution(&resolution.value.git, buf), - .github => this.verifyGitResolution(&resolution.value.github, buf), - else => this.verifyPackageJSONNameAndVersion(), + .git => this.verifyGitResolution(&resolution.value.git, buf, root_node_modules_dir), + .github => this.verifyGitResolution(&resolution.value.github, buf, root_node_modules_dir), + else => this.verifyPackageJSONNameAndVersion(root_node_modules_dir), }; } - fn verifyPackageJSONNameAndVersion(this: *PackageInstall) bool { + fn verifyPackageJSONNameAndVersion(this: *PackageInstall, root_node_modules_dir: std.fs.Dir) bool { const allocator = this.allocator; var total: usize = 0; var read: usize = 0; @@ -1018,7 +1028,12 @@ pub const PackageInstall = struct { const package_json_path: [:0]u8 = this.destination_dir_subpath_buf[0 .. this.destination_dir_subpath.len + std.fs.path.sep_str.len + "package.json".len :0]; defer this.destination_dir_subpath_buf[this.destination_dir_subpath.len] = 0; - var package_json_file = File.openat(this.destination_dir, package_json_path, std.os.O.RDONLY, 0).unwrap() catch return false; + var destination_dir = this.node_modules.openDir(root_node_modules_dir) catch return false; + defer { + if (std.fs.cwd().fd != destination_dir.fd) destination_dir.close(); + } + + var package_json_file = File.openat(destination_dir, package_json_path, std.os.O.RDONLY, 0).unwrap() catch return false; defer package_json_file.close(); // Heuristic: most package.jsons will be less than 2048 bytes. @@ -1151,7 +1166,7 @@ pub const PackageInstall = struct { else Method.hardlink; - fn installWithClonefileEachDir(this: *PackageInstall) !Result { + fn installWithClonefileEachDir(this: *PackageInstall, destination_dir: std.fs.Dir) !Result { var cached_package_dir = bun.openDir(this.cache_dir, this.cache_dir_subpath) catch |err| return Result{ .fail = .{ .err = err, .step = .opening_cache_dir }, }; @@ -1212,7 +1227,7 @@ pub const PackageInstall = struct { } }; - var subdir = this.destination_dir.makeOpenPath(bun.span(this.destination_dir_subpath), .{}) catch |err| return Result{ + var subdir = destination_dir.makeOpenPath(bun.span(this.destination_dir_subpath), .{}) catch |err| return Result{ .fail = .{ .err = err, .step = .opening_dest_dir }, }; @@ -1231,14 +1246,14 @@ pub const PackageInstall = struct { } // https://www.unix.com/man-page/mojave/2/fclonefileat/ - fn installWithClonefile(this: *PackageInstall) !Result { + fn installWithClonefile(this: *PackageInstall, destination_dir: std.fs.Dir) !Result { if (comptime !Environment.isMac) @compileError("clonefileat() is macOS only."); if (this.destination_dir_subpath[0] == '@') { if (strings.indexOfCharZ(this.destination_dir_subpath, std.fs.path.sep)) |slash| { this.destination_dir_subpath_buf[slash] = 0; const subdir = this.destination_dir_subpath_buf[0..slash :0]; - this.destination_dir.makeDirZ(subdir) catch {}; + destination_dir.makeDirZ(subdir) catch {}; this.destination_dir_subpath_buf[slash] = std.fs.path.sep; } } @@ -1246,7 +1261,7 @@ pub const PackageInstall = struct { return switch (C.clonefileat( this.cache_dir.fd, this.cache_dir_subpath, - this.destination_dir.fd, + destination_dir.fd, this.destination_dir_subpath, 0, )) { @@ -1259,7 +1274,7 @@ pub const PackageInstall = struct { // But, this can happen if this package contains a node_modules folder // We want to continue installing as many packages as we can, so we shouldn't block while downloading // We use the slow path in this case - .EXIST => try this.installWithClonefileEachDir(), + .EXIST => try this.installWithClonefileEachDir(destination_dir), .ACCES => return error.AccessDenied, else => error.Unexpected, }, @@ -1286,8 +1301,8 @@ pub const PackageInstall = struct { threadlocal var node_fs_for_package_installer: bun.JSC.Node.NodeFS = .{}; - fn initInstallDir(this: *PackageInstall, state: *InstallDirState) Result { - const destbase = this.destination_dir; + fn initInstallDir(this: *PackageInstall, state: *InstallDirState, destination_dir: std.fs.Dir) Result { + const destbase = destination_dir; const destpath = this.destination_dir_subpath; state.cached_package_dir = bun.openDir(this.cache_dir, this.cache_dir_subpath) catch |err| return Result{ @@ -1298,10 +1313,7 @@ pub const PackageInstall = struct { this.allocator, &[_]bun.OSPathSlice{}, &[_]bun.OSPathSlice{}, - ) catch |err| { - state.cached_package_dir.close(); - return Result.fail(err, .opening_cache_dir); - }; + ) catch bun.outOfMemory(); if (!Environment.isWindows) { state.subdir = destbase.makeOpenPath(bun.span(destpath), .{ @@ -1364,9 +1376,9 @@ pub const PackageInstall = struct { return Result.success(); } - fn installWithCopyfile(this: *PackageInstall) Result { + fn installWithCopyfile(this: *PackageInstall, destination_dir: std.fs.Dir) Result { var state = InstallDirState{}; - const res = this.initInstallDir(&state); + const res = this.initInstallDir(&state, destination_dir); if (res.isFail()) return res; defer state.deinit(); @@ -1642,9 +1654,9 @@ pub const PackageInstall = struct { } }; - fn installWithHardlink(this: *PackageInstall) !Result { + fn installWithHardlink(this: *PackageInstall, dest_dir: std.fs.Dir) !Result { var state = InstallDirState{}; - const res = this.initInstallDir(&state); + const res = this.initInstallDir(&state, dest_dir); if (res.isFail()) return res; defer state.deinit(); @@ -1745,9 +1757,9 @@ pub const PackageInstall = struct { }; } - fn installWithSymlink(this: *PackageInstall) !Result { + fn installWithSymlink(this: *PackageInstall, dest_dir: std.fs.Dir) !Result { var state = InstallDirState{}; - const res = this.initInstallDir(&state); + const res = this.initInstallDir(&state, dest_dir); if (res.isFail()) return res; defer state.deinit(); @@ -1887,29 +1899,14 @@ pub const PackageInstall = struct { }; } - pub fn uninstall(this: *PackageInstall) void { - this.destination_dir.deleteTree(bun.span(this.destination_dir_subpath)) catch {}; + pub fn uninstall(this: *PackageInstall, destination_dir: std.fs.Dir) void { + destination_dir.deleteTree(bun.span(this.destination_dir_subpath)) catch {}; } - pub fn uninstallBeforeInstall(this: *PackageInstall) void { - // TODO(dylan-conway): depth first package installation to allow lifecycle scripts to start earlier - // - // if (this.install_order == .depth_first) { - // var subpath_dir = this.destination_dir.open(this.destination_dir_subpath, .{}) catch return; - // defer subpath_dir.close(); - // var iter = subpath_dir.iterateAssumeFirstIteration(); - // while (iter.next() catch null) |entry| { - // // skip node_modules because installation is depth first - // if (entry.kind != .directory or !strings.eqlComptime(entry.name, "node_modules")) { - // this.destination_dir.deleteTree(entry.name) catch {}; - // } - // } - // } else { - // this.destination_dir.deleteTree(bun.span(this.destination_dir_subpath)) catch {}; - // } + pub fn uninstallBeforeInstall(this: *PackageInstall, destination_dir: std.fs.Dir) void { var rand_path_buf: [48]u8 = undefined; const temp_path = std.fmt.bufPrintZ(&rand_path_buf, ".old-{}", .{std.fmt.fmtSliceHexUpper(std.mem.asBytes(&bun.fastRandom()))}) catch unreachable; - switch (bun.sys.renameat(bun.toFD(this.destination_dir), this.destination_dir_subpath, bun.toFD(this.destination_dir), temp_path)) { + switch (bun.sys.renameat(bun.toFD(destination_dir), this.destination_dir_subpath, bun.toFD(destination_dir), temp_path)) { .err => { // if it fails, that means the directory doesn't exist or was inaccessible }, @@ -1944,7 +1941,7 @@ pub const PackageInstall = struct { var unintall_task = @fieldParentPtr(@This(), "task", task); var debug_timer = bun.Output.DebugTimer.start(); defer { - _ = PackageManager.instance.pending_tasks.fetchSub(1, .Monotonic); + _ = PackageManager.instance.decrementPendingTasks(); PackageManager.instance.wake(); } @@ -1986,8 +1983,7 @@ pub const PackageInstall = struct { .absolute_path = bun.default_allocator.dupeZ(u8, bun.path.joinAbsString(FileSystem.instance.top_level_dir, &.{ this.node_modules.path.items, temp_path }, .auto)) catch bun.outOfMemory(), }); PackageManager.instance.thread_pool.schedule(bun.ThreadPool.Batch.from(&task.task)); - _ = PackageManager.instance.pending_tasks.fetchAdd(1, .Monotonic); - PackageManager.instance.total_tasks += 1; + _ = PackageManager.instance.incrementPendingTasks(1); }, } } @@ -2043,11 +2039,11 @@ pub const PackageInstall = struct { return false; } - pub fn installFromLink(this: *PackageInstall, skip_delete: bool) Result { + pub fn installFromLink(this: *PackageInstall, skip_delete: bool, destination_dir: std.fs.Dir) Result { const dest_path = this.destination_dir_subpath; // If this fails, we don't care. // we'll catch it the next error - if (!skip_delete and !strings.eqlComptime(dest_path, ".")) this.uninstallBeforeInstall(); + if (!skip_delete and !strings.eqlComptime(dest_path, ".")) this.uninstallBeforeInstall(destination_dir); const subdir = std.fs.path.dirname(dest_path); @@ -2066,7 +2062,7 @@ pub const PackageInstall = struct { // When we're linking on Windows, we want to avoid keeping the source directory handle open if (comptime Environment.isWindows) { var wbuf: bun.WPathBuffer = undefined; - const dest_path_length = bun.windows.kernel32.GetFinalPathNameByHandleW(this.destination_dir.fd, &wbuf, dest_buf.len, 0); + const dest_path_length = bun.windows.kernel32.GetFinalPathNameByHandleW(destination_dir.fd, &wbuf, dest_buf.len, 0); if (dest_path_length == 0) { const e = bun.windows.Win32Error.get(); const err = if (e.toSystemErrno()) |sys_err| bun.errnoToZigErr(sys_err) else error.Unexpected; @@ -2104,16 +2100,16 @@ pub const PackageInstall = struct { const dest_z = dest_buf[0..offset :0]; to_buf[to_path.len] = 0; - const to_path_z = to_buf[0..to_path.len :0]; + const target_z = to_buf[0..to_path.len :0]; // https://github.com/npm/cli/blob/162c82e845d410ede643466f9f8af78a312296cc/workspaces/arborist/lib/arborist/reify.js#L738 // https://github.com/npm/cli/commit/0e58e6f6b8f0cd62294642a502c17561aaf46553 - switch (bun.sys.symlinkOrJunctionOnWindows(to_path_z, dest_z)) { + switch (bun.sys.symlinkOrJunctionOnWindows(dest_z, target_z)) { .err => |err_| brk: { var err = err_; if (err.getErrno() == .EXIST) { - _ = bun.sys.unlink(to_path_z); - switch (bun.sys.symlinkOrJunctionOnWindows(to_path_z, dest_z)) { + _ = bun.sys.unlink(target_z); + switch (bun.sys.symlinkOrJunctionOnWindows(dest_z, target_z)) { .err => |e| err = e, .result => break :brk, } @@ -2130,13 +2126,13 @@ pub const PackageInstall = struct { } } else { var dest_dir = if (subdir) |dir| brk: { - break :brk bun.MakePath.makeOpenPath(this.destination_dir, dir, .{}) catch |err| return Result{ + break :brk bun.MakePath.makeOpenPath(destination_dir, dir, .{}) catch |err| return Result{ .fail = .{ .err = err, .step = .linking, }, }; - } else this.destination_dir; + } else destination_dir; defer { if (subdir != null) dest_dir.close(); } @@ -2169,16 +2165,30 @@ pub const PackageInstall = struct { }; } - pub fn install(this: *PackageInstall, skip_delete: bool) Result { + pub fn getInstallMethod(this: *const PackageInstall) Method { + return if (strings.eqlComptime(this.cache_dir_subpath, ".") or strings.hasPrefixComptime(this.cache_dir_subpath, "..")) + Method.symlink + else + supported_method; + } + + pub fn packageMissingFromCache(this: *PackageInstall, manager: *PackageManager, package_id: PackageID) bool { + return switch (manager.getPreinstallState(package_id)) { + .done => false, + else => brk: { + const exists = Syscall.directoryExistsAt(this.cache_dir.fd, this.cache_dir_subpath).unwrap() catch false; + if (exists) manager.setPreinstallState(package_id, manager.lockfile, .done); + break :brk !exists; + }, + }; + } + pub fn install(this: *PackageInstall, skip_delete: bool, destination_dir: std.fs.Dir) Result { // If this fails, we don't care. // we'll catch it the next error - if (!skip_delete and !strings.eqlComptime(this.destination_dir_subpath, ".")) this.uninstallBeforeInstall(); + if (!skip_delete and !strings.eqlComptime(this.destination_dir_subpath, ".")) this.uninstallBeforeInstall(destination_dir); - var supported_method_to_use = if (strings.eqlComptime(this.cache_dir_subpath, ".") or strings.hasPrefixComptime(this.cache_dir_subpath, "..")) - Method.symlink - else - supported_method; + var supported_method_to_use = this.getInstallMethod(); switch (supported_method_to_use) { .clonefile => { @@ -2186,7 +2196,7 @@ pub const PackageInstall = struct { // First, attempt to use clonefile // if that fails due to ENOTSUP, mark it as unsupported and then fall back to copyfile - if (this.installWithClonefile()) |result| { + if (this.installWithClonefile(destination_dir)) |result| { return result; } else |err| { switch (err) { @@ -2206,7 +2216,7 @@ pub const PackageInstall = struct { }, .clonefile_each_dir => { if (comptime Environment.isMac) { - if (this.installWithClonefileEachDir()) |result| { + if (this.installWithClonefileEachDir(destination_dir)) |result| { return result; } else |err| { switch (err) { @@ -2225,7 +2235,7 @@ pub const PackageInstall = struct { } }, .hardlink => { - if (this.installWithHardlink()) |result| { + if (this.installWithHardlink(destination_dir)) |result| { return result; } else |err| outer: { if (comptime !Environment.isWindows) { @@ -2250,7 +2260,7 @@ pub const PackageInstall = struct { if (comptime Environment.isWindows) { supported_method_to_use = .copyfile; } else { - if (this.installWithSymlink()) |result| { + if (this.installWithSymlink(destination_dir)) |result| { return result; } else |err| { switch (err) { @@ -2272,69 +2282,25 @@ pub const PackageInstall = struct { }; // TODO: linux io_uring - return this.installWithCopyfile(); + return this.installWithCopyfile(destination_dir); } }; -const NodeModulesFolder = struct { - fd: ?bun.FileDescriptor = null, +pub const Resolution = @import("./resolution.zig").Resolution; +const Progress = std.Progress; +const TaggedPointer = @import("../tagged_pointer.zig"); + +const DependencyInstallContext = struct { tree_id: Lockfile.Tree.Id = 0, path: std.ArrayList(u8) = std.ArrayList(u8).init(bun.default_allocator), - - pub fn deinit(this: *NodeModulesFolder) void { - if (this.fd) |fd| { - this.fd = null; - if (std.fs.cwd().fd == fd.cast()) { - return; - } - - _ = bun.sys.close(fd); - } - - this.path.clearAndFree(); - } - - pub fn dir(this: *NodeModulesFolder, root: std.fs.Dir) !std.fs.Dir { - if (this.fd) |fd| { - return fd.asDir(); - } - - if (root.fd == std.fs.cwd().fd) { - this.fd = bun.toFD(std.fs.cwd()); - return root; - } - - const out = brk: { - if (comptime Environment.isPosix) { - break :brk try root.makeOpenPath(this.path.items, .{ .iterate = true, .access_sub_paths = true }); - } - - try bun.MakePath.makePath(u8, root, this.path.items); - break :brk (try bun.sys.openDirAtWindowsA(bun.toFD(root), this.path.items, .{ - .can_rename_or_delete = false, - .create = true, - .read_only = false, - }).unwrap()).asDir(); - }; - this.fd = bun.toFD(out.fd); - return out; - } + dependency_id: DependencyID, }; -pub const Resolution = @import("./resolution.zig").Resolution; -const Progress = std.Progress; -const TaggedPointer = @import("../tagged_pointer.zig"); -const TaskCallbackContext = union(Tag) { +const TaskCallbackContext = union(enum) { dependency: DependencyID, - node_modules_folder: NodeModulesFolder, + dependency_install_context: DependencyInstallContext, root_dependency: DependencyID, root_request_id: PackageID, - pub const Tag = enum { - dependency, - node_modules_folder, - root_dependency, - root_request_id, - }; }; const TaskCallbackList = std.ArrayListUnmanaged(TaskCallbackContext); @@ -2808,17 +2774,6 @@ pub const PackageManager = struct { return this.global_link_dir_path; } - fn ensurePreinstallStateListCapacity(this: *PackageManager, count: usize) !void { - if (this.preinstall_state.items.len >= count) { - return; - } - - const offset = this.preinstall_state.items.len; - try this.preinstall_state.ensureTotalCapacity(this.allocator, count); - this.preinstall_state.expandToCapacity(); - @memset(this.preinstall_state.items[offset..], PreinstallState.unknown); - } - pub fn formatLaterVersionInCache( this: *PackageManager, name: []const u8, @@ -2860,19 +2815,30 @@ pub const PackageManager = struct { } } + fn ensurePreinstallStateListCapacity(this: *PackageManager, count: usize) !void { + if (this.preinstall_state.items.len >= count) { + return; + } + + const offset = this.preinstall_state.items.len; + try this.preinstall_state.ensureTotalCapacity(this.allocator, count); + this.preinstall_state.expandToCapacity(); + @memset(this.preinstall_state.items[offset..], PreinstallState.unknown); + } + pub fn setPreinstallState(this: *PackageManager, package_id: PackageID, lockfile: *Lockfile, value: PreinstallState) void { this.ensurePreinstallStateListCapacity(lockfile.packages.len) catch return; this.preinstall_state.items[package_id] = value; } - pub fn getPreinstallState(this: *PackageManager, package_id: PackageID, _: *Lockfile) PreinstallState { + pub fn getPreinstallState(this: *PackageManager, package_id: PackageID) PreinstallState { if (package_id >= this.preinstall_state.items.len) { return PreinstallState.unknown; } return this.preinstall_state.items[package_id]; } pub fn determinePreinstallState(manager: *PackageManager, this: Package, lockfile: *Lockfile) PreinstallState { - switch (manager.getPreinstallState(this.meta.id, lockfile)) { + switch (manager.getPreinstallState(this.meta.id)) { .unknown => { // Do not automatically start downloading packages which are disabled @@ -4303,7 +4269,7 @@ pub const PackageManager = struct { } if (result.network_task) |network_task| { - if (this.getPreinstallState(result.package.meta.id, this.lockfile) == .extract) { + if (this.getPreinstallState(result.package.meta.id) == .extract) { this.setPreinstallState(result.package.meta.id, this.lockfile, .extracting); this.enqueueNetworkTask(network_task); } @@ -4799,8 +4765,7 @@ pub const PackageManager = struct { pub fn scheduleTasks(manager: *PackageManager) usize { const count = manager.task_batch.len + manager.network_resolve_batch.len + manager.network_tarball_batch.len; - _ = manager.pending_tasks.fetchAdd(@truncate(count), .Monotonic); - manager.total_tasks += @as(u32, @truncate(count)); + _ = manager.incrementPendingTasks(@truncate(count)); manager.thread_pool.schedule(manager.task_batch); manager.network_resolve_batch.push(manager.network_tarball_batch); HTTP.http_thread.schedule(manager.network_resolve_batch); @@ -5196,7 +5161,7 @@ pub const PackageManager = struct { var network_tasks_iter = network_tasks_batch.iterator(); while (network_tasks_iter.next()) |task| { if (comptime Environment.allow_assert) bun.assert(manager.pendingTaskCount() > 0); - _ = manager.pending_tasks.fetchSub(1, .Monotonic); + _ = manager.decrementPendingTasks(); // We cannot free the network task at the end of this scope. // It may continue to be referenced in a future task. @@ -5522,7 +5487,7 @@ pub const PackageManager = struct { while (resolve_tasks_iter.next()) |task| { if (comptime Environment.allow_assert) bun.assert(manager.pendingTaskCount() > 0); defer manager.preallocated_resolve_tasks.put(task); - _ = manager.pending_tasks.fetchSub(1, .Monotonic); + _ = manager.decrementPendingTasks(); if (task.log.msgs.items.len > 0) { switch (Output.enable_ansi_colors) { @@ -8688,6 +8653,43 @@ pub const PackageManager = struct { } } + pub const NodeModulesFolder = struct { + tree_id: Lockfile.Tree.Id = 0, + path: std.ArrayList(u8) = std.ArrayList(u8).init(bun.default_allocator), + + pub fn deinit(this: *NodeModulesFolder) void { + this.path.clearAndFree(); + } + + pub fn openDir(this: *const NodeModulesFolder, root: std.fs.Dir) !std.fs.Dir { + if (comptime Environment.isPosix) { + return root.openDir(this.path.items, .{ .iterate = true, .access_sub_paths = true }); + } + + return (try bun.sys.openDirAtWindowsA(bun.toFD(root), this.path.items, .{ + .can_rename_or_delete = false, + .create = true, + .read_only = false, + }).unwrap()).asDir(); + } + + pub fn makeAndOpenDir(this: *NodeModulesFolder, root: std.fs.Dir) !std.fs.Dir { + const out = brk: { + if (comptime Environment.isPosix) { + break :brk try root.makeOpenPath(this.path.items, .{ .iterate = true, .access_sub_paths = true }); + } + + try bun.MakePath.makePath(u8, root, this.path.items); + break :brk (try bun.sys.openDirAtWindowsA(bun.toFD(root), this.path.items, .{ + .can_rename_or_delete = false, + .create = true, + .read_only = false, + }).unwrap()).asDir(); + }; + return out; + } + }; + pub const PackageInstaller = struct { manager: *PackageManager, lockfile: *Lockfile, @@ -8728,11 +8730,18 @@ pub const PackageManager = struct { tree_id: Lockfile.Tree.Id, }) = .{}, + pending_installs_to_tree_id: []std.ArrayListUnmanaged(DependencyInstallContext), + trusted_dependencies_from_update_requests: std.AutoArrayHashMapUnmanaged(TruncatedPackageNameHash, void), /// Increments the number of installed packages for a tree id and runs available scripts /// if the tree is finished. - pub fn incrementTreeInstallCount(this: *PackageInstaller, tree_id: Lockfile.Tree.Id, comptime log_level: Options.LogLevel) void { + pub fn incrementTreeInstallCount( + this: *PackageInstaller, + tree_id: Lockfile.Tree.Id, + comptime should_install_packages: bool, + comptime log_level: Options.LogLevel, + ) void { if (comptime Environment.allow_assert) { bun.assert(tree_id != Lockfile.Tree.invalid_id); } @@ -8754,6 +8763,10 @@ pub const PackageManager = struct { if (!is_not_done) { this.completed_trees.set(tree_id); + if (comptime should_install_packages) { + const force = false; + this.installAvailablePackages(log_level, force); + } this.runAvailableScripts(log_level); } } @@ -8795,6 +8808,51 @@ pub const PackageManager = struct { } } + pub fn installAvailablePackages(this: *PackageInstaller, comptime log_level: Options.LogLevel, comptime force: bool) void { + const prev_node_modules = this.node_modules; + defer this.node_modules = prev_node_modules; + const prev_tree_id = this.current_tree_id; + defer this.current_tree_id = prev_tree_id; + + const lockfile = this.lockfile; + const resolutions = lockfile.buffers.resolutions.items; + + for (this.pending_installs_to_tree_id, 0..) |*pending_installs, i| { + if (force or this.canInstallPackageForTree(this.lockfile.buffers.trees.items, @intCast(i))) { + defer pending_installs.clearRetainingCapacity(); + + // If installing these packages completes the tree, we don't allow it + // to call `installAvailablePackages` recursively. Starting at id 0 and + // going up ensures we will reach any trees that will be able to install + // packages upon completing the current tree + for (pending_installs.items) |context| { + const package_id = resolutions[context.dependency_id]; + const name = lockfile.str(&this.names[package_id]); + const resolution = &this.resolutions[package_id]; + this.node_modules.tree_id = context.tree_id; + this.node_modules.path = context.path; + this.current_tree_id = context.tree_id; + + const needs_verify = false; + const is_pending_package_install = true; + this.installPackageWithNameAndResolution( + // This id might be different from the id used to enqueue the task. Important + // to use the correct one because the package might be aliased with a different + // name + context.dependency_id, + package_id, + log_level, + name, + resolution, + needs_verify, + is_pending_package_install, + ); + this.node_modules.deinit(); + } + } + } + } + pub fn completeRemainingScripts(this: *PackageInstaller, comptime log_level: Options.LogLevel) void { for (this.pending_lifecycle_scripts.items) |entry| { const package_name = entry.list.package_name; @@ -8856,6 +8914,17 @@ pub const PackageManager = struct { LifecycleScriptSubprocess.alive_count.load(.Monotonic) < this.manager.options.max_concurrent_lifecycle_scripts; } + /// If all parents of the tree have finished installing their packages, the package can be installed + pub fn canInstallPackageForTree(this: *const PackageInstaller, trees: []Lockfile.Tree, package_tree_id: Lockfile.Tree.Id) bool { + var curr_tree_id = trees[package_tree_id].parent; + while (curr_tree_id != Lockfile.Tree.invalid_id) { + if (!this.completed_trees.isSet(curr_tree_id)) return false; + curr_tree_id = trees[curr_tree_id].parent; + } + + return true; + } + // pub fn printTreeDeps(this: *PackageInstaller) void { // for (this.tree_ids_to_trees_the_id_depends_on, 0..) |deps, j| { // std.debug.print("tree #{d:3}: ", .{j}); @@ -8869,6 +8938,10 @@ pub const PackageManager = struct { pub fn deinit(this: *PackageInstaller) void { const allocator = this.manager.allocator; this.pending_lifecycle_scripts.deinit(this.manager.allocator); + for (this.pending_installs_to_tree_id) |*pending_installs| { + pending_installs.deinit(this.manager.allocator); + } + this.manager.allocator.free(this.pending_installs_to_tree_id); this.completed_trees.deinit(allocator); allocator.free(this.tree_install_counts); this.tree_ids_to_trees_the_id_depends_on.deinit(allocator); @@ -8919,28 +8992,32 @@ pub const PackageManager = struct { } for (callbacks.items) |*cb| { - this.node_modules = cb.node_modules_folder; - var dir = this.node_modules.dir(this.root_node_modules_folder) catch |err| { - if (log_level != .silent) { - Output.err(err, "Failed to open node_modules folder for {s} in {s}", .{ name, bun.fmt.fmtPath(u8, this.node_modules.path.items, .{}) }); - } - this.summary.fail += 1; - continue; - }; - defer dir.close(); - this.node_modules.fd = null; - this.current_tree_id = cb.node_modules_folder.tree_id; - cb.node_modules_folder = .{}; + const context = cb.dependency_install_context; + const callback_package_id = this.lockfile.buffers.resolutions.items[context.dependency_id]; + const callback_resolution = &this.resolutions[callback_package_id]; + this.node_modules.tree_id = context.tree_id; + this.node_modules.path = context.path; + this.current_tree_id = context.tree_id; + const needs_verify = false; + const is_pending_package_install = false; this.installPackageWithNameAndResolution( - dependency_id, - package_id, + // This id might be different from the id used to enqueue the task. Important + // to use the correct one because the package might be aliased with a different + // name + context.dependency_id, + callback_package_id, log_level, name, - resolution, - dir, + callback_resolution, + needs_verify, + is_pending_package_install, ); this.node_modules.deinit(); } + } else { + if (comptime Environment.allow_assert) { + Output.panic("Ran callback to install enqueued packages, but there was no task associated with it. {d} {any}", .{ dependency_id, data.* }); + } } } @@ -9033,7 +9110,14 @@ pub const PackageManager = struct { comptime log_level: Options.LogLevel, name: string, resolution: *const Resolution, - destination_dir: std.fs.Dir, + + // false when coming from download. if the package was downloaded + // it was already determined to need an install + comptime needs_verify: bool, + + // we don't want to allow more package installs through + // pending packages if we're already draining them. + comptime is_pending_package_install: bool, ) void { const buf = this.lockfile.buffers.string_bytes.items; @@ -9052,7 +9136,6 @@ pub const PackageManager = struct { .progress = this.progress, .cache_dir = undefined, .cache_dir_subpath = undefined, - .destination_dir = destination_dir, .destination_dir_subpath = destination_dir_subpath, .destination_dir_subpath_buf = &this.destination_dir_subpath_buf, .allocator = this.lockfile.allocator, @@ -9133,6 +9216,7 @@ pub const PackageManager = struct { Output.flush(); this.summary.fail += 1; + this.incrementTreeInstallCount(this.current_tree_id, !is_pending_package_install, log_level); return; }; @@ -9163,21 +9247,128 @@ pub const PackageManager = struct { if (comptime Environment.allow_assert) { @panic("bad"); } - this.incrementTreeInstallCount(this.current_tree_id, log_level); + this.incrementTreeInstallCount(this.current_tree_id, !is_pending_package_install, log_level); return; }, } - const needs_install = this.force_install or this.skip_verify_installed_version_number or !installer.verify(resolution, buf); + const needs_install = this.force_install or this.skip_verify_installed_version_number or !needs_verify or !installer.verify( + resolution, + buf, + this.root_node_modules_folder, + ); this.summary.skipped += @intFromBool(!needs_install); if (needs_install) { - const result: PackageInstall.Result = switch (resolution.tag) { - .symlink, .workspace => installer.installFromLink(this.skip_delete), - else => installer.install(this.skip_delete), + if (resolution.tag.canEnqueueInstallTask() and installer.packageMissingFromCache(this.manager, package_id)) { + if (comptime Environment.allow_assert) { + bun.assert(resolution.canEnqueueInstallTask()); + } + + const context: TaskCallbackContext = .{ + .dependency_install_context = .{ + .tree_id = this.current_tree_id, + .path = this.node_modules.path.clone() catch bun.outOfMemory(), + .dependency_id = dependency_id, + }, + }; + switch (resolution.tag) { + .git => { + this.manager.enqueueGitForCheckout( + dependency_id, + alias, + resolution, + context, + ); + }, + .github => { + const url = this.manager.allocGitHubURL(&resolution.value.github); + defer this.manager.allocator.free(url); + this.manager.enqueueTarballForDownload( + dependency_id, + package_id, + url, + context, + ); + }, + .local_tarball => { + this.manager.enqueueTarballForReading( + dependency_id, + alias, + resolution, + context, + ); + }, + .remote_tarball => { + this.manager.enqueueTarballForDownload( + dependency_id, + package_id, + resolution.value.remote_tarball.slice(buf), + context, + ); + }, + .npm => { + if (comptime Environment.isDebug) { + // Very old versions of Bun didn't store the tarball url when it didn't seem necessary + // This caused bugs. We can't assert on it because they could come from old lockfiles + if (resolution.value.npm.url.isEmpty()) { + Output.debugWarn("package {s}@{} missing tarball_url", .{ name, resolution.fmt(buf, .posix) }); + } + } + + this.manager.enqueuePackageForDownload( + name, + dependency_id, + package_id, + resolution.value.npm.version, + resolution.value.npm.url.slice(buf), + context, + ); + }, + else => { + if (comptime Environment.allow_assert) { + @panic("unreachable, handled above"); + } + this.incrementTreeInstallCount(this.current_tree_id, !is_pending_package_install, log_level); + this.summary.fail += 1; + }, + } + + return; + } + + if (!is_pending_package_install and !this.canInstallPackageForTree(this.lockfile.buffers.trees.items, this.current_tree_id)) { + this.pending_installs_to_tree_id[this.current_tree_id].append(this.manager.allocator, .{ + .dependency_id = dependency_id, + .tree_id = this.current_tree_id, + .path = this.node_modules.path.clone() catch bun.outOfMemory(), + }) catch bun.outOfMemory(); + return; + } + + // creating this directory now, right before installing package + var destination_dir = this.node_modules.makeAndOpenDir(this.root_node_modules_folder) catch |err| { + if (log_level != .silent) { + Output.err(err, "Failed to open node_modules folder for {s} in {s}", .{ + name, + bun.fmt.fmtPath(u8, this.node_modules.path.items, .{}), + }); + } + this.summary.fail += 1; + this.incrementTreeInstallCount(this.current_tree_id, !is_pending_package_install, log_level); + return; + }; + + defer { + if (std.fs.cwd().fd != destination_dir.fd) destination_dir.close(); + } + + const install_result = switch (resolution.tag) { + .symlink, .workspace => installer.installFromLink(this.skip_delete, destination_dir), + else => installer.install(this.skip_delete, destination_dir), }; - switch (result) { + switch (install_result) { .success => { const is_duplicate = this.successfully_installed.isSet(package_id); this.summary.success += @as(u32, @intFromBool(!is_duplicate)); @@ -9223,7 +9414,7 @@ pub const PackageManager = struct { } if (this.manager.options.enable.fail_early) { - installer.uninstall(); + installer.uninstall(destination_dir); Global.crash(); } } @@ -9276,140 +9467,91 @@ pub const PackageManager = struct { } } - this.incrementTreeInstallCount(this.current_tree_id, log_level); + this.incrementTreeInstallCount(this.current_tree_id, !is_pending_package_install, log_level); }, .fail => |cause| { - if (cause.isPackageMissingFromCache()) { - const context: TaskCallbackContext = .{ - .node_modules_folder = .{ - .fd = null, - .tree_id = this.current_tree_id, - .path = this.node_modules.path.clone() catch bun.outOfMemory(), - }, + if (comptime Environment.allow_assert) { + bun.assert(!cause.isPackageMissingFromCache() or (resolution.tag != .symlink and resolution.tag != .workspace)); + } + + // even if the package failed to install, we still need to increment the install + // counter for this tree + this.incrementTreeInstallCount(this.current_tree_id, !is_pending_package_install, log_level); + + if (cause.err == error.DanglingSymlink) { + Output.prettyErrorln( + "error: {s} \"link:{s}\" not found (try running 'bun link' in the intended package's folder)", + .{ @errorName(cause.err), this.names[package_id].slice(buf) }, + ); + this.summary.fail += 1; + } else if (cause.err == error.AccessDenied) { + // there are two states this can happen + // - Access Denied because node_modules/ is unwritable + // - Access Denied because this specific package is unwritable + // in the case of the former, the logs are extremely noisy, so we + // will exit early, otherwise set a flag to not re-stat + const Singleton = struct { + var node_modules_is_ok = false; }; - switch (resolution.tag) { - .git => { - this.manager.enqueueGitForCheckout( - dependency_id, - alias, - resolution, - context, - ); - }, - .github => { - const url = this.manager.allocGitHubURL(&resolution.value.github); - defer this.manager.allocator.free(url); - this.manager.enqueueTarballForDownload( - dependency_id, - package_id, - url, - context, - ); - }, - .local_tarball => { - this.manager.enqueueTarballForReading( - dependency_id, - alias, - resolution, - context, - ); - }, - .remote_tarball => { - this.manager.enqueueTarballForDownload( - dependency_id, - package_id, - resolution.value.remote_tarball.slice(buf), - context, - ); - }, - .npm => { - if (comptime Environment.isDebug) { - // Very old versions of Bun didn't store the tarball url when it didn't seem necessary - // This caused bugs. We can't assert on it because they could come from old lockfiles - if (resolution.value.npm.url.isEmpty()) { - Output.debugWarn("package {s}@{} missing tarball_url", .{ name, resolution.fmt(buf, .posix) }); + if (!Singleton.node_modules_is_ok) { + if (!Environment.isWindows) { + const stat = bun.sys.fstat(bun.toFD(destination_dir)).unwrap() catch |err| { + Output.err("EACCES", "Permission denied while installing {s}", .{ + this.names[package_id].slice(buf), + }); + if (Environment.isDebug) { + Output.err(err, "Failed to stat node_modules", .{}); } - } - - this.manager.enqueuePackageForDownload( - name, - dependency_id, - package_id, - resolution.value.npm.version, - resolution.value.npm.url.slice(buf), - context, - ); - }, - else => { - Output.prettyErrorln( - "error: {s} installing {s} ({s})", - .{ @errorName(cause.err), this.names[package_id].slice(buf), result.fail.step.name() }, - ); - this.summary.fail += 1; - }, - } - } else { - // even if the package failed to install, we still need to increment the install - // counter for this tree - this.incrementTreeInstallCount(this.current_tree_id, log_level); - if (cause.err == error.DanglingSymlink) { - Output.prettyErrorln( - "error: {s} \"link:{s}\" not found (try running 'bun link' in the intended package's folder)", - .{ @errorName(cause.err), this.names[package_id].slice(buf) }, - ); - this.summary.fail += 1; - } else if (cause.err == error.AccessDenied) { - // there are two states this can happen - // - Access Denied because node_modules/ is unwritable - // - Access Denied because this specific package is unwritable - // in the case of the former, the logs are extremely noisy, so we - // will exit early, otherwise set a flag to not re-stat - const Singleton = struct { - var node_modules_is_ok = false; - }; - if (!Singleton.node_modules_is_ok) { - if (!Environment.isWindows) { - const stat = bun.sys.fstat(bun.toFD(destination_dir)).unwrap() catch |err| { - Output.err("EACCES", "Permission denied while installing {s}", .{ - this.names[package_id].slice(buf), - }); - if (Environment.isDebug) { - Output.err(err, "Failed to stat node_modules", .{}); - } - Global.exit(1); - }; + Global.exit(1); + }; - const is_writable = if (stat.uid == bun.C.getuid()) - stat.mode & bun.S.IWUSR > 0 - else if (stat.gid == bun.C.getgid()) - stat.mode & bun.S.IWGRP > 0 - else - stat.mode & bun.S.IWOTH > 0; + const is_writable = if (stat.uid == bun.C.getuid()) + stat.mode & bun.S.IWUSR > 0 + else if (stat.gid == bun.C.getgid()) + stat.mode & bun.S.IWGRP > 0 + else + stat.mode & bun.S.IWOTH > 0; - if (!is_writable) { - Output.err("EACCES", "Permission denied while writing packages into node_modules.", .{}); - Global.exit(1); - } + if (!is_writable) { + Output.err("EACCES", "Permission denied while writing packages into node_modules.", .{}); + Global.exit(1); } - Singleton.node_modules_is_ok = true; } + Singleton.node_modules_is_ok = true; + } - Output.err("EACCES", "Permission denied while installing {s}", .{ - this.names[package_id].slice(buf), - }); + Output.err("EACCES", "Permission denied while installing {s}", .{ + this.names[package_id].slice(buf), + }); - this.summary.fail += 1; - } else { - Output.prettyErrorln( - "error: {s} installing {s} ({s})", - .{ @errorName(cause.err), this.names[package_id].slice(buf), result.fail.step.name() }, - ); - this.summary.fail += 1; - } + this.summary.fail += 1; + } else { + Output.prettyErrorln( + "error: {s} installing {s} ({s})", + .{ @errorName(cause.err), this.names[package_id].slice(buf), install_result.fail.step.name() }, + ); + this.summary.fail += 1; } }, } } else { + defer this.incrementTreeInstallCount(this.current_tree_id, !is_pending_package_install, log_level); + + var destination_dir = this.node_modules.makeAndOpenDir(this.root_node_modules_folder) catch |err| { + if (log_level != .silent) { + Output.err(err, "Failed to open node_modules folder for {s} in {s}", .{ + name, + bun.fmt.fmtPath(u8, this.node_modules.path.items, .{}), + }); + } + this.summary.fail += 1; + return; + }; + + defer { + if (std.fs.cwd().fd != destination_dir.fd) destination_dir.close(); + } + const name_hash: TruncatedPackageNameHash = @truncate(this.lockfile.buffers.dependencies.items[dependency_id].name_hash); const is_trusted, const is_trusted_through_update_request, const add_to_lockfile = brk: { // trusted through a --trust dependency. need to enqueue scripts, write to package.json, and add to lockfile @@ -9515,24 +9657,33 @@ pub const PackageManager = struct { pub fn installPackage( this: *PackageInstaller, dependency_id: DependencyID, - destination_dir: std.fs.Dir, comptime log_level: Options.LogLevel, ) void { const package_id = this.lockfile.buffers.resolutions.items[dependency_id]; const meta = &this.metas[package_id]; + const is_pending_package_install = false; if (meta.isDisabled()) { if (comptime log_level.showProgress()) { this.node.completeOne(); } - this.incrementTreeInstallCount(this.current_tree_id, log_level); + this.incrementTreeInstallCount(this.current_tree_id, !is_pending_package_install, log_level); return; } const name = this.lockfile.str(&this.names[package_id]); const resolution = &this.resolutions[package_id]; - this.installPackageWithNameAndResolution(dependency_id, package_id, log_level, name, resolution, destination_dir); + const needs_verify = true; + this.installPackageWithNameAndResolution( + dependency_id, + package_id, + log_level, + name, + resolution, + needs_verify, + is_pending_package_install, + ); } }; @@ -9780,15 +9931,6 @@ pub const PackageManager = struct { if (options.enable.force_install) { skip_verify_installed_version_number = true; skip_delete = false; - - // TODO(dylan-conway): depth first installation - // var node_modules_iter = node_modules_folder.iterateAssumeFirstIteration(); - // defer node_modules_iter.reset(); - // while (try node_modules_iter.next()) |entry| { - // if (entry.kind != .directory or !strings.eqlComptime(entry.name, ".cache")) { - // node_modules_folder.deleteTree(entry.name) catch {}; - // } - // } } var summary = PackageInstall.Summary{}; @@ -9804,8 +9946,9 @@ pub const PackageManager = struct { // to make mistakes harder var parts = this.lockfile.packages.slice(); + const trees = this.lockfile.buffers.trees.items; + const completed_trees, const tree_ids_to_trees_the_id_depends_on, const tree_install_counts = trees: { - const trees = this.lockfile.buffers.trees.items; const completed_trees = try Bitset.initEmpty(this.allocator, trees.len); var tree_ids_to_trees_the_id_depends_on = try Bitset.List.initEmpty(this.allocator, trees.len, trees.len); @@ -9852,6 +9995,17 @@ pub const PackageManager = struct { }; }; + // Each tree (other than the root tree) can accumulate packages it cannot install until + // each of it's parent trees have installed their packages. We keep arrays of these pending + // packages for each tree, and drain them when a tree is completed (each of it's immediate + // dependencies are installed). + // + // Trees are drained breadth first because if the current tree is completed from + // the remaining pending installs, then any child tree has a higher chance of + // being able to install it's dependencies + const pending_installs_to_tree_id = this.allocator.alloc(std.ArrayListUnmanaged(DependencyInstallContext), trees.len) catch bun.outOfMemory(); + @memset(pending_installs_to_tree_id, .{}); + const trusted_dependencies_from_update_requests: std.AutoArrayHashMapUnmanaged(TruncatedPackageNameHash, void) = trusted_deps: { // find all deps originating from --trust packages from cli @@ -9892,7 +10046,6 @@ pub const PackageManager = struct { .lockfile = this.lockfile, .node = &install_node, .node_modules = .{ - .fd = bun.toFD(node_modules_folder.fd), .path = std.ArrayList(u8).fromOwnedSlice( this.allocator, try this.allocator.dupe( @@ -9918,6 +10071,7 @@ pub const PackageManager = struct { .completed_trees = completed_trees, .tree_install_counts = tree_install_counts, .trusted_dependencies_from_update_requests = trusted_dependencies_from_update_requests, + .pending_installs_to_tree_id = pending_installs_to_tree_id, }; }; @@ -9934,12 +10088,6 @@ pub const PackageManager = struct { var remaining = node_modules.dependencies; installer.current_tree_id = node_modules.tree_id; - var destination_dir = try installer.node_modules.dir(node_modules_folder); - defer { - installer.node_modules.fd = null; - destination_dir.close(); - } - if (comptime Environment.allow_assert) { bun.assert(node_modules.dependencies.len == this.lockfile.buffers.trees.items[installer.current_tree_id].dependencies.len); } @@ -9952,7 +10100,7 @@ pub const PackageManager = struct { while (remaining.len > unroll_count) { comptime var i: usize = 0; inline while (i < unroll_count) : (i += 1) { - installer.installPackage(remaining[i], destination_dir, comptime log_level); + installer.installPackage(remaining[i], comptime log_level); } remaining = remaining[unroll_count..]; @@ -9978,7 +10126,7 @@ pub const PackageManager = struct { } for (remaining) |dependency_id| { - installer.installPackage(dependency_id, destination_dir, log_level); + installer.installPackage(dependency_id, log_level); } try this.runTasks( @@ -10049,6 +10197,14 @@ pub const PackageManager = struct { this.tickLifecycleScripts(); } + for (installer.pending_installs_to_tree_id) |pending_installs| { + if (comptime Environment.allow_assert) { + bun.assert(pending_installs.items.len == 0); + } + const force = true; + installer.installAvailablePackages(log_level, force); + } + this.finished_installing.store(true, .Monotonic); if (comptime log_level.showProgress()) { scripts_node.activate(); @@ -10080,6 +10236,15 @@ pub const PackageManager = struct { return manager.pending_tasks.load(.Monotonic); } + pub inline fn incrementPendingTasks(manager: *PackageManager, count: u32) u32 { + manager.total_tasks += count; + return manager.pending_tasks.fetchAdd(count, .Monotonic); + } + + pub inline fn decrementPendingTasks(manager: *PackageManager) u32 { + return manager.pending_tasks.fetchSub(1, .Monotonic); + } + pub fn setupGlobalDir(manager: *PackageManager, ctx: Command.Context) !void { manager.options.global_bin_dir = try Options.openGlobalBinDir(ctx.install); var out_buffer: [bun.MAX_PATH_BYTES]u8 = undefined; diff --git a/src/install/lockfile.zig b/src/install/lockfile.zig index 75f3629eeac33..e03c400b43b8d 100644 --- a/src/install/lockfile.zig +++ b/src/install/lockfile.zig @@ -738,6 +738,13 @@ pub fn cleanWithLogger( // We will only shrink the number of packages here. // never grow + // preinstall_state is used during installPackages. the indexes(package ids) need + // to be remapped + var preinstall_state = PackageManager.instance.preinstall_state; + var old_preinstall_state = preinstall_state.clone(old.allocator) catch bun.outOfMemory(); + defer old_preinstall_state.deinit(old.allocator); + @memset(preinstall_state.items, .unknown); + if (updates.len > 0) { try old.preprocessUpdateRequests(updates, exact_versions); } @@ -775,6 +782,7 @@ pub fn cleanWithLogger( .mapping = package_id_mapping, .clone_queue = clone_queue_, .log = log, + .old_preinstall_state = old_preinstall_state, }; // try clone_queue.ensureUnusedCapacity(root.dependencies.len); @@ -925,6 +933,7 @@ const Cloner = struct { trees: Tree.List = Tree.List{}, trees_count: u32 = 1, log: *logger.Log, + old_preinstall_state: std.ArrayListUnmanaged(Install.PreinstallState), pub fn flush(this: *Cloner) anyerror!void { const max_package_id = this.old.packages.len; @@ -2841,6 +2850,10 @@ pub const Package = extern struct { package_id_mapping[this.meta.id] = new_package.meta.id; + if (PackageManager.instance.preinstall_state.items.len > 0) { + PackageManager.instance.preinstall_state.items[new_package.meta.id] = cloner.old_preinstall_state.items[this.meta.id]; + } + for (old_dependencies, dependencies) |old_dep, *new_dep| { new_dep.* = try old_dep.clone( old_string_buf, diff --git a/src/install/resolution.zig b/src/install/resolution.zig index f50cf2c6f0ac0..555b5951f4547 100644 --- a/src/install/resolution.zig +++ b/src/install/resolution.zig @@ -28,6 +28,10 @@ pub const Resolution = extern struct { return this.tag.isGit(); } + pub fn canEnqueueInstallTask(this: *const Resolution) bool { + return this.tag.canEnqueueInstallTask(); + } + pub fn order( lhs: *const Resolution, rhs: *const Resolution, @@ -337,5 +341,9 @@ pub const Resolution = extern struct { pub fn isGit(this: Tag) bool { return this == .git or this == .github or this == .gitlab; } + + pub fn canEnqueueInstallTask(this: Tag) bool { + return this == .npm or this == .local_tarball or this == .remote_tarball or this == .git or this == .github; + } }; }; diff --git a/src/resolver/resolver.zig b/src/resolver/resolver.zig index 92afb977cec59..1d87d1e2026c3 100644 --- a/src/resolver/resolver.zig +++ b/src/resolver/resolver.zig @@ -1935,7 +1935,7 @@ pub const Resolver = struct { const dir_path_for_resolution = manager.pathForResolution(resolved_package_id, resolution, bufs(.path_in_global_disk_cache)) catch |err| { // if it's missing, we need to install it if (err == error.FileNotFound) { - switch (manager.getPreinstallState(resolved_package_id, manager.lockfile)) { + switch (manager.getPreinstallState(resolved_package_id)) { .done => { var path = Fs.Path.init(import_path); path.is_disabled = true; diff --git a/src/sys.zig b/src/sys.zig index 270369fb97cc2..457e309aae579 100644 --- a/src/sys.zig +++ b/src/sys.zig @@ -1733,11 +1733,11 @@ pub const WindowsSymlinkOptions = packed struct { pub var has_failed_to_create_symlink = false; }; -pub fn symlinkOrJunctionOnWindows(sym: [:0]const u8, target: [:0]const u8) Maybe(void) { +pub fn symlinkOrJunctionOnWindows(dest: [:0]const u8, target: [:0]const u8) Maybe(void) { if (!WindowsSymlinkOptions.has_failed_to_create_symlink) { var sym16: bun.WPathBuffer = undefined; var target16: bun.WPathBuffer = undefined; - const sym_path = bun.strings.toNTPath(&sym16, sym); + const sym_path = bun.strings.toNTPath(&sym16, dest); const target_path = bun.strings.toNTPath(&target16, target); switch (symlinkW(sym_path, target_path, .{ .directory = true })) { .result => { @@ -1751,17 +1751,17 @@ pub fn symlinkOrJunctionOnWindows(sym: [:0]const u8, target: [:0]const u8) Maybe } } - return sys_uv.symlinkUV(sym, target, bun.windows.libuv.UV_FS_SYMLINK_JUNCTION); + return sys_uv.symlinkUV(target, dest, bun.windows.libuv.UV_FS_SYMLINK_JUNCTION); } -pub fn symlinkW(sym: [:0]const u16, target: [:0]const u16, options: WindowsSymlinkOptions) Maybe(void) { +pub fn symlinkW(dest: [:0]const u16, target: [:0]const u16, options: WindowsSymlinkOptions) Maybe(void) { while (true) { const flags = options.flags(); - if (windows.kernel32.CreateSymbolicLinkW(sym, target, flags) == 0) { + if (windows.kernel32.CreateSymbolicLinkW(dest, target, flags) == 0) { const errno = bun.windows.Win32Error.get(); log("CreateSymbolicLinkW({}, {}, {any}) = {s}", .{ - bun.fmt.fmtPath(u16, sym, .{}), + bun.fmt.fmtPath(u16, dest, .{}), bun.fmt.fmtPath(u16, target, .{}), flags, @tagName(errno), @@ -1788,7 +1788,7 @@ pub fn symlinkW(sym: [:0]const u16, target: [:0]const u16, options: WindowsSymli } log("CreateSymbolicLinkW({}, {}, {any}) = 0", .{ - bun.fmt.fmtPath(u16, sym, .{}), + bun.fmt.fmtPath(u16, dest, .{}), bun.fmt.fmtPath(u16, target, .{}), flags, }); @@ -2160,14 +2160,14 @@ pub fn directoryExistsAt(dir_: anytype, subpath: anytype) JSC.Maybe(bool) { } if (comptime !has_sentinel) { - const path = std.os.toPosixPath(subpath) catch return JSC.Maybe(bool){ .err = Error.oom }; + const path = std.os.toPosixPath(subpath) catch return JSC.Maybe(bool){ .err = Error.fromCode(.NAMETOOLONG, .access) }; return directoryExistsAt(dir_fd, path); } if (comptime Environment.isLinux) { // avoid loading the libc symbol for this to reduce chances of GLIBC minimum version requirements - const rc = linux.faccessat(dir_fd.cast(), subpath, linux.O.DIRECTORY | linux.O.RDONLY, 0); - syslog("faccessat({}, {}, O_DIRECTORY | O_RDONLY, 0) = {d}", .{ dir_fd, bun.fmt.fmtOSPath(subpath, .{}), rc }); + const rc = linux.faccessat(dir_fd.cast(), subpath, linux.F_OK, 0); + syslog("faccessat({}, {}, O_DIRECTORY | O_RDONLY, 0) = {d}", .{ dir_fd, bun.fmt.fmtOSPath(subpath, .{}), if (rc == 0) 0 else @intFromEnum(linux.getErrno(rc)) }); if (rc == 0) { return JSC.Maybe(bool){ .result = true }; } @@ -2175,9 +2175,9 @@ pub fn directoryExistsAt(dir_: anytype, subpath: anytype) JSC.Maybe(bool) { return JSC.Maybe(bool){ .result = false }; } - // on toher platforms use faccessat from libc - const rc = std.c.faccessat(dir_fd.cast(), subpath, std.os.O.DIRECTORY | std.os.O.RDONLY, 0); - syslog("faccessat({}, {}, O_DIRECTORY | O_RDONLY, 0) = {d}", .{ dir_fd, bun.fmt.fmtOSPath(subpath, .{}), rc }); + // on other platforms use faccessat from libc + const rc = std.c.faccessat(dir_fd.cast(), subpath, std.os.F_OK, 0); + syslog("faccessat({}, {}, O_DIRECTORY | O_RDONLY, 0) = {d}", .{ dir_fd, bun.fmt.fmtOSPath(subpath, .{}), if (rc == 0) 0 else @intFromEnum(std.c.getErrno(rc)) }); if (rc == 0) { return JSC.Maybe(bool){ .result = true }; } @@ -2187,7 +2187,7 @@ pub fn directoryExistsAt(dir_: anytype, subpath: anytype) JSC.Maybe(bool) { pub fn existsAt(fd: bun.FileDescriptor, subpath: []const u8) bool { if (comptime Environment.isPosix) { - return system.faccessat(bun.toFD(fd), &(std.os.toPosixPath(subpath) catch return false), 0, 0) == 0; + return system.faccessat(fd.cast(), &(std.os.toPosixPath(subpath) catch return false), 0, 0) == 0; } if (comptime Environment.isWindows) { From fbe2fe0c3fd198530db51d9f54e0a03ffff9f22d Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Mon, 15 Apr 2024 21:32:17 -0700 Subject: [PATCH 4/8] Bump --- CMakeLists.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index eaf7b30e710cb..7c20793ea28ae 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.22) cmake_policy(SET CMP0091 NEW) cmake_policy(SET CMP0067 NEW) -set(Bun_VERSION "1.1.3") +set(Bun_VERSION "1.1.4") set(WEBKIT_TAG e3a2d89a0b1644cc8d5c245bd2ffee4d4bd6c1d5) set(BUN_WORKDIR "${CMAKE_CURRENT_BINARY_DIR}") @@ -555,6 +555,7 @@ else() add_compile_definitions("BUN_DEBUG=1") set(ASSERT_ENABLED "1") endif() + message(STATUS "Using WebKit from ${WEBKIT_DIR}") else() if(NOT EXISTS "${WEBKIT_DIR}/lib/${libWTF}.${STATIC_LIB_EXT}" OR NOT EXISTS "${WEBKIT_DIR}/lib/${libJavaScriptCore}.${STATIC_LIB_EXT}") From 291a39bd3f4b1ffd987a35f110ea619c1ec0972d Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Tue, 16 Apr 2024 14:03:02 -0700 Subject: [PATCH 5/8] Do not run shell tests outside test scope (#10199) * Do not run tests outside test scope * Fix tests * Fix type errors and remove potentially precarious uses of unreachable * yoops * Remove all instances of "Ruh roh" --------- Co-authored-by: Zack Radisic <56137411+zackradisic@users.noreply.github.com> --- src/js/private.d.ts | 2 +- src/shell/interpreter.zig | 20 +- test/js/bun/shell/bunshell.test.ts | 15 +- test/js/bun/shell/commands/basename.test.ts | 3 +- test/js/bun/shell/commands/dirname.test.ts | 3 +- test/js/bun/shell/commands/exit.test.ts | 3 +- test/js/bun/shell/commands/false.test.ts | 3 +- test/js/bun/shell/commands/mv.test.ts | 3 +- test/js/bun/shell/commands/rm.test.ts | 3 +- test/js/bun/shell/commands/seq.test.ts | 3 +- test/js/bun/shell/commands/true.test.ts | 3 +- test/js/bun/shell/env.positionals.test.ts | 3 +- test/js/bun/shell/exec.test.ts | 3 +- test/js/bun/shell/leak.test.ts | 14 +- test/js/bun/shell/lex.test.ts | 3 +- test/js/bun/shell/parse.test.ts | 3 +- test/js/bun/shell/test_builder.ts | 493 ++++++++++---------- test/js/bun/shell/util.ts | 4 +- 18 files changed, 315 insertions(+), 269 deletions(-) diff --git a/src/js/private.d.ts b/src/js/private.d.ts index 9b4474c031b46..2b8d7d5bf6b70 100644 --- a/src/js/private.d.ts +++ b/src/js/private.d.ts @@ -105,7 +105,7 @@ declare module "bun" { var TOML: { parse(contents: string): any; }; - function jest(): typeof import("bun:test"); + function jest(path: string): typeof import("bun:test"); var main: string; var tty: Array<{ hasColors: boolean }>; var FFI: any; diff --git a/src/shell/interpreter.zig b/src/shell/interpreter.zig index 85185028184bc..d60d04ea3ab37 100644 --- a/src/shell/interpreter.zig +++ b/src/shell/interpreter.zig @@ -58,7 +58,7 @@ const stderr_no = 2; pub fn OOM(e: anyerror) noreturn { if (comptime bun.Environment.allow_assert) { - if (e != error.OutOfMemory) @panic("Ruh roh"); + if (e != error.OutOfMemory) bun.outOfMemory(); } @panic("Out of memory"); } @@ -2123,7 +2123,7 @@ pub const Interpreter = struct { return; } - unreachable; + @panic("Invalid child to Expansion, this indicates a bug in Bun. Please file a report on Github."); } fn onGlobWalkDone(this: *Expansion, task: *ShellGlobTask) void { @@ -2742,7 +2742,7 @@ pub const Interpreter = struct { return; } - unreachable; + @panic("Invalid child to Assigns expression, this indicates a bug in Bun. Please file a report on Github."); } }; @@ -2910,7 +2910,7 @@ pub const Interpreter = struct { parent: ParentPtr, io: IO, ) *Binary { - var binary = interpreter.allocator.create(Binary) catch |err| std.debug.panic("Ruh roh: {any}\n", .{err}); + var binary = interpreter.allocator.create(Binary) catch bun.outOfMemory(); binary.node = node; binary.base = .{ .kind = .binary, .interpreter = interpreter, .shell = shell_state }; binary.parent = parent; @@ -3234,7 +3234,7 @@ pub const Interpreter = struct { if (ptr == @as(usize, @intCast(child.ptr.repr._ptr))) break :brk i; } } - unreachable; + @panic("Invalid pipeline state"); }; log("pipeline child done {x} ({d}) i={d}", .{ @intFromPtr(this), exit_code, idx }); @@ -4347,7 +4347,7 @@ pub const Interpreter = struct { parent: ParentPtr, io: IO, ) *Cmd { - var cmd = interpreter.allocator.create(Cmd) catch |err| std.debug.panic("Ruh roh: {any}\n", .{err}); + var cmd = interpreter.allocator.create(Cmd) catch bun.outOfMemory(); cmd.* = .{ .base = .{ .kind = .cmd, .interpreter = interpreter, .shell = shell_state }, .node = node, @@ -4522,7 +4522,8 @@ pub const Interpreter = struct { this.next(); return; } - unreachable; + + @panic("Expected Cmd child to be Assigns or Expansion. This indicates a bug in Bun. Please file a GitHub issue. "); } fn initSubproc(this: *Cmd) void { @@ -7115,7 +7116,8 @@ pub const Interpreter = struct { while (!(this.state == .err or this.state == .done)) { switch (this.state) { .waiting_io => return, - .idle, .done, .err => unreachable, + .idle => @panic("Unexpected \"idle\" state in Pwd. This indicates a bug in Bun. Please file a GitHub issue."), + .done, .err => unreachable, } } @@ -9680,7 +9682,7 @@ pub const Interpreter = struct { pub fn next(this: *Exit) void { switch (this.state) { - .idle => unreachable, + .idle => @panic("Unexpected \"idle\" state in Exit. This indicates a bug in Bun. Please file a GitHub issue."), .waiting_io => { return; }, diff --git a/test/js/bun/shell/bunshell.test.ts b/test/js/bun/shell/bunshell.test.ts index b69fa71006b80..3fbe3da4b2710 100644 --- a/test/js/bun/shell/bunshell.test.ts +++ b/test/js/bun/shell/bunshell.test.ts @@ -10,7 +10,8 @@ import { mkdir, mkdtemp, realpath, rm, stat } from "fs/promises"; import { bunEnv, bunExe, runWithErrorPromise, tempDirWithFiles } from "harness"; import { tmpdir } from "os"; import { join, sep } from "path"; -import { TestBuilder, sortedShellOutput } from "./util"; +import { createTestBuilder, sortedShellOutput } from "./util"; +const TestBuilder = createTestBuilder(import.meta.path); $.env(bunEnv); $.cwd(process.cwd()); @@ -797,9 +798,10 @@ describe("deno_task", () => { TestBuilder.command`echo 1 | echo 2 && echo 3`.stdout("2\n3\n").runAsTest("pipe in conditional"); - await TestBuilder.command`echo $(sleep 0.1 && echo 2 & echo 1) | BUN_DEBUG_QUIET_LOGS=1 BUN_TEST_VAR=1 ${BUN} -e 'await process.stdin.pipe(process.stdout)'` + TestBuilder.command`echo $(sleep 0.1 && echo 2 & echo 1) | BUN_DEBUG_QUIET_LOGS=1 BUN_TEST_VAR=1 ${BUN} -e 'await process.stdin.pipe(process.stdout)'` .stdout("1 2\n") - .run(); + .todo("& not supported") + .runAsTest("complicated pipeline"); TestBuilder.command`echo 2 | echo 1 | BUN_TEST_VAR=1 ${BUN} -e 'process.stdin.pipe(process.stdout)'` .stdout("1\n") @@ -834,9 +836,12 @@ describe("deno_task", () => { }); describe("redirects", async function igodf() { - await TestBuilder.command`echo 5 6 7 > test.txt`.fileEquals("test.txt", "5 6 7\n").run(); + TestBuilder.command`echo 5 6 7 > test.txt`.fileEquals("test.txt", "5 6 7\n").runAsTest("basic redirect"); - await TestBuilder.command`echo 1 2 3 && echo 1 > test.txt`.stdout("1 2 3\n").fileEquals("test.txt", "1\n").run(); + TestBuilder.command`echo 1 2 3 && echo 1 > test.txt` + .stdout("1 2 3\n") + .fileEquals("test.txt", "1\n") + .runAsTest("basic redirect with &&"); // subdir TestBuilder.command`mkdir subdir && cd subdir && echo 1 2 3 > test.txt` diff --git a/test/js/bun/shell/commands/basename.test.ts b/test/js/bun/shell/commands/basename.test.ts index 93ebd3a59c125..44c77ba78597a 100644 --- a/test/js/bun/shell/commands/basename.test.ts +++ b/test/js/bun/shell/commands/basename.test.ts @@ -1,6 +1,7 @@ import { $ } from "bun"; import { describe, test, expect } from "bun:test"; -import { TestBuilder } from "../test_builder"; +import { createTestBuilder } from "../test_builder"; +const TestBuilder = createTestBuilder(import.meta.path); describe("basename", async () => { TestBuilder.command`basename`.exitCode(1).stdout("").stderr("usage: basename string\n").runAsTest("shows usage"); diff --git a/test/js/bun/shell/commands/dirname.test.ts b/test/js/bun/shell/commands/dirname.test.ts index a5f9f2b92a25f..f054d01ce9261 100644 --- a/test/js/bun/shell/commands/dirname.test.ts +++ b/test/js/bun/shell/commands/dirname.test.ts @@ -1,6 +1,7 @@ import { $ } from "bun"; import { describe, test, expect } from "bun:test"; -import { TestBuilder } from "../test_builder"; +import { createTestBuilder } from "../test_builder"; +const TestBuilder = createTestBuilder(import.meta.path); describe("dirname", async () => { TestBuilder.command`dirname`.exitCode(1).stdout("").stderr("usage: dirname string\n").runAsTest("shows usage"); diff --git a/test/js/bun/shell/commands/exit.test.ts b/test/js/bun/shell/commands/exit.test.ts index 241a847a8d46e..5e757a9c80a4d 100644 --- a/test/js/bun/shell/commands/exit.test.ts +++ b/test/js/bun/shell/commands/exit.test.ts @@ -1,6 +1,7 @@ import { $ } from "bun"; import { describe, test, expect } from "bun:test"; -import { TestBuilder } from "../test_builder"; +import { createTestBuilder } from "../test_builder"; +const TestBuilder = createTestBuilder(import.meta.path); import { sortedShellOutput } from "../util"; import { join } from "path"; diff --git a/test/js/bun/shell/commands/false.test.ts b/test/js/bun/shell/commands/false.test.ts index f07de9d8fd375..785a495ee99c2 100644 --- a/test/js/bun/shell/commands/false.test.ts +++ b/test/js/bun/shell/commands/false.test.ts @@ -1,6 +1,7 @@ import { $ } from "bun"; import { describe, test, expect } from "bun:test"; -import { TestBuilder } from "../test_builder"; +import { createTestBuilder } from "../test_builder"; +const TestBuilder = createTestBuilder(import.meta.path); describe("false", async () => { TestBuilder.command`false`.exitCode(1).runAsTest("works"); diff --git a/test/js/bun/shell/commands/mv.test.ts b/test/js/bun/shell/commands/mv.test.ts index ceea68aca2cb0..0a4fdec5201b5 100644 --- a/test/js/bun/shell/commands/mv.test.ts +++ b/test/js/bun/shell/commands/mv.test.ts @@ -1,6 +1,7 @@ import { $ } from "bun"; import { describe, test, expect } from "bun:test"; -import { TestBuilder } from "../test_builder"; +import { createTestBuilder } from "../test_builder"; +const TestBuilder = createTestBuilder(import.meta.path); import { sortedShellOutput } from "../util"; import { join } from "path"; diff --git a/test/js/bun/shell/commands/rm.test.ts b/test/js/bun/shell/commands/rm.test.ts index cff064032413c..51987e5984b25 100644 --- a/test/js/bun/shell/commands/rm.test.ts +++ b/test/js/bun/shell/commands/rm.test.ts @@ -10,7 +10,8 @@ import { $ } from "bun"; import path from "path"; import { mkdirSync, writeFileSync } from "node:fs"; import { ShellOutput } from "bun"; -import { TestBuilder, sortedShellOutput } from "../util"; +import { createTestBuilder, sortedShellOutput } from "../util"; +const TestBuilder = createTestBuilder(import.meta.path); const fileExists = async (path: string): Promise => $`ls -d ${path}`.then(o => o.stdout.toString() === `${path}\n`); diff --git a/test/js/bun/shell/commands/seq.test.ts b/test/js/bun/shell/commands/seq.test.ts index 7052d11f88326..54504faa8bd91 100644 --- a/test/js/bun/shell/commands/seq.test.ts +++ b/test/js/bun/shell/commands/seq.test.ts @@ -1,6 +1,7 @@ import { $ } from "bun"; import { describe } from "bun:test"; -import { TestBuilder } from "../test_builder"; +import { createTestBuilder } from "../test_builder"; +const TestBuilder = createTestBuilder(import.meta.path); describe("seq", async () => { TestBuilder.command`seq` diff --git a/test/js/bun/shell/commands/true.test.ts b/test/js/bun/shell/commands/true.test.ts index 490ab741f99b5..4dc491713e63c 100644 --- a/test/js/bun/shell/commands/true.test.ts +++ b/test/js/bun/shell/commands/true.test.ts @@ -1,6 +1,7 @@ import { $ } from "bun"; import { describe, test, expect } from "bun:test"; -import { TestBuilder } from "../test_builder"; +import { createTestBuilder } from "../test_builder"; +const TestBuilder = createTestBuilder(import.meta.path); describe("true", async () => { TestBuilder.command`true`.exitCode(0).runAsTest("works"); diff --git a/test/js/bun/shell/env.positionals.test.ts b/test/js/bun/shell/env.positionals.test.ts index 802900be1dfac..407c54b1842cc 100644 --- a/test/js/bun/shell/env.positionals.test.ts +++ b/test/js/bun/shell/env.positionals.test.ts @@ -1,6 +1,7 @@ import { $, spawn } from "bun"; import { describe, test, expect } from "bun:test"; -import { TestBuilder } from "./test_builder"; +import { createTestBuilder } from "./test_builder"; +const TestBuilder = createTestBuilder(import.meta.path); import { bunEnv, bunExe } from "harness"; import * as path from "node:path"; diff --git a/test/js/bun/shell/exec.test.ts b/test/js/bun/shell/exec.test.ts index 49ecbf5b10fa6..498add1fff59d 100644 --- a/test/js/bun/shell/exec.test.ts +++ b/test/js/bun/shell/exec.test.ts @@ -1,6 +1,7 @@ import { $ } from "bun"; import { describe, test, expect } from "bun:test"; -import { TestBuilder } from "./test_builder"; +import { createTestBuilder } from "./test_builder"; +const TestBuilder = createTestBuilder(import.meta.path); import { bunEnv } from "harness"; const BUN = process.argv0; diff --git a/test/js/bun/shell/leak.test.ts b/test/js/bun/shell/leak.test.ts index fbfb4992a3720..980510c86585a 100644 --- a/test/js/bun/shell/leak.test.ts +++ b/test/js/bun/shell/leak.test.ts @@ -4,7 +4,9 @@ import { bunEnv } from "harness"; import { appendFileSync, closeSync, openSync, writeFileSync } from "node:fs"; import { tmpdir, devNull } from "os"; import { join } from "path"; -import { TestBuilder } from "./util"; +import { createTestBuilder } from "./util"; +const TestBuilder = createTestBuilder(import.meta.path); +type TestBuilder = InstanceType; $.env(bunEnv); $.cwd(process.cwd()); @@ -50,7 +52,7 @@ const TESTS: [name: string, builder: () => TestBuilder, runs?: number][] = [ ]; describe("fd leak", () => { - function fdLeakTest(name: string, builder: () => TestBuilder, runs: number = 500, threshold: number = 5) { + function fdLeakTest(name: string, builder: () => TestBuilder, runs: number = 1000, threshold: number = 5) { test(`fdleak_${name}`, async () => { Bun.gc(true); const baseline = openSync(devNull, "r"); @@ -83,13 +85,15 @@ describe("fd leak", () => { writeFileSync(tempfile, testcode); const impl = /* ts */ ` + const TestBuilder = createTestBuilder(import.meta.path); + const threshold = ${threshold} let prev: number | undefined = undefined; let prevprev: number | undefined = undefined; for (let i = 0; i < ${runs}; i++) { Bun.gc(true); await (async function() { - await ${builder.toString().slice("() =>".length)}.quiet().run() + await ${builder.toString().slice("() =>".length)}.quiet().runAsTest('iter:', i) })() Bun.gc(true); Bun.gc(true); @@ -111,7 +115,9 @@ describe("fd leak", () => { env: bunEnv, }); // console.log('STDOUT:', stdout.toString(), '\n\nSTDERR:', stderr.toString()); - console.log("\n\nSTDERR:", stderr.toString()); + if (exitCode != 0) { + console.log("\n\nSTDERR:", stderr.toString()); + } expect(exitCode).toBe(0); }, 100_000); } diff --git a/test/js/bun/shell/lex.test.ts b/test/js/bun/shell/lex.test.ts index 88b5cd883723c..b3bb1ba5c5722 100644 --- a/test/js/bun/shell/lex.test.ts +++ b/test/js/bun/shell/lex.test.ts @@ -1,7 +1,8 @@ import { $ } from "bun"; -import { TestBuilder, redirect } from "./util"; +import { createTestBuilder, redirect } from "./util"; import { shellInternals } from "bun:internal-for-testing"; const { lex } = shellInternals; +const TestBuilder = createTestBuilder(import.meta.path); const BUN = process.argv0; diff --git a/test/js/bun/shell/parse.test.ts b/test/js/bun/shell/parse.test.ts index dbbb4dea21ce0..0f42686f68354 100644 --- a/test/js/bun/shell/parse.test.ts +++ b/test/js/bun/shell/parse.test.ts @@ -1,6 +1,7 @@ -import { TestBuilder, redirect } from "./util"; +import { createTestBuilder, redirect } from "./util"; import { shellInternals } from "bun:internal-for-testing"; const { parse } = shellInternals; +const TestBuilder = createTestBuilder(import.meta.path); describe("parse shell", () => { test("basic", () => { diff --git a/test/js/bun/shell/test_builder.ts b/test/js/bun/shell/test_builder.ts index f9eaee87dce5f..a43aa762a681d 100644 --- a/test/js/bun/shell/test_builder.ts +++ b/test/js/bun/shell/test_builder.ts @@ -1,4 +1,3 @@ -import { describe, test, afterAll, beforeAll, expect } from "bun:test"; import { ShellError, ShellOutput } from "bun"; import { ShellPromise } from "bun"; // import { tempDirWithFiles } from "harness"; @@ -6,34 +5,45 @@ import { join } from "node:path"; import * as os from "node:os"; import * as fs from "node:fs"; -export class TestBuilder { - private promise: { type: "ok"; val: ShellPromise } | { type: "err"; val: Error }; - private _testName: string | undefined = undefined; +export function createTestBuilder(path: string) { + var { describe, test, afterAll, beforeAll, expect, beforeEach, afterEach } = Bun.jest(path); - private expected_stdout: string | ((stdout: string, tempdir: string) => void) = ""; - private expected_stderr: string | ((stderr: string, tempdir: string) => void) = ""; - private expected_exit_code: number = 0; - private expected_error: ShellError | string | boolean | undefined = undefined; - private file_equals: { [filename: string]: string } = {}; - private _doesNotExist: string[] = []; - private _timeout: number | undefined = undefined; + var insideTestScope = false; + beforeEach(() => { + insideTestScope = true; + }); + afterEach(() => { + insideTestScope = false; + }); - private tempdir: string | undefined = undefined; - private _env: { [key: string]: string } | undefined = undefined; + class TestBuilder { + promise: { type: "ok"; val: ShellPromise } | { type: "err"; val: Error }; + _testName: string | undefined = undefined; - private __todo: boolean | string = false; + expected_stdout: string | ((stdout: string, tempdir: string) => void) = ""; + expected_stderr: string | ((stderr: string, tempdir: string) => void) = ""; + expected_exit_code: number = 0; + expected_error: ShellError | string | boolean | undefined = undefined; + file_equals: { [filename: string]: string } = {}; + _doesNotExist: string[] = []; + _timeout: number | undefined = undefined; - static UNEXPECTED_SUBSHELL_ERROR_OPEN = - "Unexpected `(`, subshells are currently not supported right now. Escape the `(` or open a GitHub issue."; + tempdir: string | undefined = undefined; + _env: { [key: string]: string } | undefined = undefined; - static UNEXPECTED_SUBSHELL_ERROR_CLOSE = - "Unexpected `)`, subshells are currently not supported right now. Escape the `)` or open a GitHub issue."; + __todo: boolean | string = false; - constructor(promise: TestBuilder["promise"]) { - this.promise = promise; - } + UNEXPECTED_SUBSHELL_ERROR_OPEN = + "Unexpected `(`, subshells are currently not supported right now. Escape the `(` or open a GitHub issue."; + + UNEXPECTED_SUBSHELL_ERROR_CLOSE = + "Unexpected `)`, subshells are currently not supported right now. Escape the `)` or open a GitHub issue."; - /** + public constructor(promise: TestBuilder["promise"]) { + this.promise = promise; + } + + /** * Start the test builder with a command: * * @example @@ -43,259 +53,270 @@ export class TestBuilder { * TestBuilder.command`echo hi!`.stdout('hi!\n').runAsTest('echo works') * ``` */ - static command(strings: TemplateStringsArray, ...expressions: any[]): TestBuilder { - try { - if (process.env.BUN_DEBUG_SHELL_LOG_CMD === "1") console.info("[ShellTestBuilder] Cmd", strings.join("")); - const promise = Bun.$(strings, ...expressions).nothrow(); - const This = new this({ type: "ok", val: promise }); - This._testName = strings.join(""); - return This; - } catch (err) { - return new this({ type: "err", val: err as Error }); + public static command(strings: TemplateStringsArray, ...expressions: any[]): TestBuilder { + try { + if (process.env.BUN_DEBUG_SHELL_LOG_CMD === "1") console.info("[ShellTestBuilder] Cmd", strings.join("")); + const promise = Bun.$(strings, ...expressions).nothrow(); + const This = new this({ type: "ok", val: promise }); + This._testName = strings.join(""); + return This; + } catch (err) { + return new this({ type: "err", val: err as Error }); + } } - } - - directory(path: string): this { - const tempdir = this.getTempDir(); - fs.mkdirSync(join(tempdir, path), { recursive: true }); - return this; - } - doesNotExist(path: string): this { - this._doesNotExist.push(path); - return this; - } + public directory(path: string): this { + const tempdir = this.getTempDir(); + fs.mkdirSync(join(tempdir, path), { recursive: true }); + return this; + } - /** - * Create a file in a temp directory - * @param path Path to the new file, this will be inside the TestBuilder's temp directory - * @param contents Contents of the new file - * @returns - * - * @example - * ```ts - * TestBuilder.command`ls .` - * .file('hi.txt', 'hi!') - * .file('hello.txt', 'hello!') - * .runAsTest('List files') - * ``` - */ - file(path: string, contents: string): this { - const tempdir = this.getTempDir(); - fs.writeFileSync(join(tempdir, path), contents); - return this; - } + doesNotExist(path: string): this { + this._doesNotExist.push(path); + return this; + } - env(env: { [key: string]: string }): this { - this._env = env; - return this; - } + /** + * Create a file in a temp directory + * @param path Path to the new file, this will be inside the TestBuilder's temp directory + * @param contents Contents of the new file + * @returns + * + * @example + * ```ts + * TestBuilder.command`ls .` + * .file('hi.txt', 'hi!') + * .file('hello.txt', 'hello!') + * .runAsTest('List files') + * ``` + */ + file(path: string, contents: string): this { + const tempdir = this.getTempDir(); + fs.writeFileSync(join(tempdir, path), contents); + return this; + } - quiet(): this { - if (this.promise.type === "ok") { - this.promise.val.quiet(); + env(env: { [key: string]: string }): this { + this._env = env; + return this; } - return this; - } - testName(name: string): this { - this._testName = name; - return this; - } + quiet(): this { + if (this.promise.type === "ok") { + this.promise.val.quiet(); + } + return this; + } - /** - * Expect output from stdout - * - * @param expected - can either be a string or a function which itself calls `expect()` - */ - stdout(expected: string | ((stdout: string, tempDir: string) => void)): this { - this.expected_stdout = expected; - return this; - } + testName(name: string): this { + this._testName = name; + return this; + } - stderr(expected: string | ((stderr: string, tempDir: string) => void)): this { - this.expected_stderr = expected; - return this; - } + /** + * Expect output from stdout + * + * @param expected - can either be a string or a function which itself calls `expect()` + */ + stdout(expected: string | ((stdout: string, tempDir: string) => void)): this { + this.expected_stdout = expected; + return this; + } - /** - * Makes this test use a temp directory: - * - The shell's cwd will be set to the temp directory - * - All FS functions on the `TestBuilder` will use this temp directory. - * @returns - */ - ensureTempDir(): this { - this.getTempDir(); - return this; - } + stderr(expected: string | ((stderr: string, tempDir: string) => void)): this { + this.expected_stderr = expected; + return this; + } - error(expected?: ShellError | string | boolean): this { - if (expected === undefined || expected === true) { - this.expected_error = true; - } else if (expected === false) { - this.expected_error = false; - } else { - this.expected_error = expected; + /** + * Makes this test use a temp directory: + * - The shell's cwd will be set to the temp directory + * - All FS functions on the `TestBuilder` will use this temp directory. + * @returns + */ + ensureTempDir(): this { + this.getTempDir(); + return this; } - return this; - } - exitCode(expected: number): this { - this.expected_exit_code = expected; - return this; - } + error(expected?: ShellError | string | boolean): this { + if (expected === undefined || expected === true) { + this.expected_error = true; + } else if (expected === false) { + this.expected_error = false; + } else { + this.expected_error = expected; + } + return this; + } - fileEquals(filename: string, expected: string): this { - this.getTempDir(); - this.file_equals[filename] = expected; - return this; - } + exitCode(expected: number): this { + this.expected_exit_code = expected; + return this; + } - static tmpdir(): string { - const tmp = os.tmpdir(); - return fs.mkdtempSync(join(tmp, "test_builder")); - } + fileEquals(filename: string, expected: string): this { + this.getTempDir(); + this.file_equals[filename] = expected; + return this; + } - setTempdir(tempdir: string): this { - this.tempdir = tempdir; - if (this.promise.type === "ok") { - this.promise.val.cwd(this.tempdir!); + static tmpdir(): string { + const tmp = os.tmpdir(); + return fs.mkdtempSync(join(tmp, "test_builder")); } - return this; - } - getTempDir(): string { - if (this.tempdir === undefined) { - this.tempdir = TestBuilder.tmpdir(); + setTempdir(tempdir: string): this { + this.tempdir = tempdir; if (this.promise.type === "ok") { this.promise.val.cwd(this.tempdir!); } - return this.tempdir!; + return this; } - return this.tempdir; - } - timeout(ms: number): this { - this._timeout = ms; - return this; - } - - async run(): Promise { - if (this.promise.type === "err") { - const err = this.promise.val; - if (this.expected_error === undefined) throw err; - if (this.expected_error === true) return undefined; - if (this.expected_error === false) expect(err).toBeUndefined(); - if (typeof this.expected_error === "string") { - expect(err.message).toEqual(this.expected_error); - } else if (this.expected_error instanceof ShellError) { - expect(err).toBeInstanceOf(ShellError); - const e = err as ShellError; - expect(e.exitCode).toEqual(this.expected_error.exitCode); - expect(e.stdout.toString()).toEqual(this.expected_error.stdout.toString()); - expect(e.stderr.toString()).toEqual(this.expected_error.stderr.toString()); + getTempDir(): string { + if (this.tempdir === undefined) { + this.tempdir = TestBuilder.tmpdir(); + if (this.promise.type === "ok") { + this.promise.val.cwd(this.tempdir!); + } + return this.tempdir!; } - return undefined; + return this.tempdir; } - const output = await (this._env !== undefined ? this.promise.val.env(this._env) : this.promise.val); + timeout(ms: number): this { + this._timeout = ms; + return this; + } - const { stdout, stderr, exitCode } = output!; - const tempdir = this.tempdir || "NO_TEMP_DIR"; - if (this.expected_stdout !== undefined) { - if (typeof this.expected_stdout === "string") { - expect(stdout.toString()).toEqual(this.expected_stdout.replaceAll("$TEMP_DIR", tempdir)); - } else { - this.expected_stdout(stdout.toString(), tempdir); + async run(): Promise { + if (!insideTestScope) { + const err = new Error("TestBuilder.run() must be called inside a test scope"); + test("TestBuilder.run() must be called inside a test scope", () => { + throw err; + }); + return Promise.resolve(undefined); } - } - if (this.expected_stderr !== undefined) { - if (typeof this.expected_stderr === "string") { - expect(stderr.toString()).toEqual(this.expected_stderr.replaceAll("$TEMP_DIR", tempdir)); - } else { - this.expected_stderr(stderr.toString(), tempdir); + + if (this.promise.type === "err") { + const err = this.promise.val; + if (this.expected_error === undefined) throw err; + if (this.expected_error === true) return undefined; + if (this.expected_error === false) expect(err).toBeUndefined(); + if (typeof this.expected_error === "string") { + expect(err.message).toEqual(this.expected_error); + } else if (this.expected_error instanceof ShellError) { + expect(err).toBeInstanceOf(ShellError); + const e = err as ShellError; + expect(e.exitCode).toEqual(this.expected_error.exitCode); + expect(e.stdout.toString()).toEqual(this.expected_error.stdout.toString()); + expect(e.stderr.toString()).toEqual(this.expected_error.stderr.toString()); + } + return undefined; } - } - if (this.expected_exit_code !== undefined) expect(exitCode).toEqual(this.expected_exit_code); - for (const [filename, expected] of Object.entries(this.file_equals)) { - const actual = await Bun.file(join(this.tempdir!, filename)).text(); - expect(actual).toEqual(expected); - } + const output = await (this._env !== undefined ? this.promise.val.env(this._env) : this.promise.val); - for (const fsname of this._doesNotExist) { - expect(fs.existsSync(join(this.tempdir!, fsname))).toBeFalsy(); - } + const { stdout, stderr, exitCode } = output!; + const tempdir = this.tempdir || "NO_TEMP_DIR"; + if (this.expected_stdout !== undefined) { + if (typeof this.expected_stdout === "string") { + expect(stdout.toString()).toEqual(this.expected_stdout.replaceAll("$TEMP_DIR", tempdir)); + } else { + this.expected_stdout(stdout.toString(), tempdir); + } + } + if (this.expected_stderr !== undefined) { + if (typeof this.expected_stderr === "string") { + expect(stderr.toString()).toEqual(this.expected_stderr.replaceAll("$TEMP_DIR", tempdir)); + } else { + this.expected_stderr(stderr.toString(), tempdir); + } + } + if (this.expected_exit_code !== undefined) expect(exitCode).toEqual(this.expected_exit_code); - // return output; - } + for (const [filename, expected] of Object.entries(this.file_equals)) { + const actual = await Bun.file(join(this.tempdir!, filename)).text(); + expect(actual).toEqual(expected); + } - todo(reason?: string): this { - this.__todo = typeof reason === "string" ? reason : true; - return this; - } + for (const fsname of this._doesNotExist) { + expect(fs.existsSync(join(this.tempdir!, fsname))).toBeFalsy(); + } - runAsTest(name: string) { - // biome-ignore lint/complexity/noUselessThisAlias: - const tb = this; - if (this.__todo) { - test.todo(typeof this.__todo === "string" ? `${name} skipped: ${this.__todo}` : name, async () => { - await tb.run(); - }); - } else { - test( - name, - async () => { + // return output; + } + + todo(reason?: string): this { + this.__todo = typeof reason === "string" ? reason : true; + return this; + } + + runAsTest(name: string) { + // biome-ignore lint/complexity/noUselessThisAlias: + const tb = this; + if (this.__todo) { + test.todo(typeof this.__todo === "string" ? `${name} skipped: ${this.__todo}` : name, async () => { await tb.run(); - }, - this._timeout, - ); + }); + } else { + test( + name, + async () => { + await tb.run(); + }, + this._timeout, + ); + } } + + // async run(): Promise { + // async function doTest(tb: TestBuilder) { + // if (tb.promise.type === "err") { + // const err = tb.promise.val; + // if (tb.expected_error === undefined) throw err; + // if (tb.expected_error === true) return undefined; + // if (tb.expected_error === false) expect(err).toBeUndefined(); + // if (typeof tb.expected_error === "string") { + // expect(err.message).toEqual(tb.expected_error); + // } + // return undefined; + // } + + // const output = await tb.promise.val; + + // const { stdout, stderr, exitCode } = output!; + // if (tb.expected_stdout !== undefined) expect(stdout.toString()).toEqual(tb.expected_stdout); + // if (tb.expected_stderr !== undefined) expect(stderr.toString()).toEqual(tb.expected_stderr); + // if (tb.expected_exit_code !== undefined) expect(exitCode).toEqual(tb.expected_exit_code); + + // for (const [filename, expected] of Object.entries(tb.file_equals)) { + // const actual = await Bun.file(filename).text(); + // expect(actual).toEqual(expected); + // } + // return output; + // } + + // if (this._testName !== undefined) { + // test(this._testName, async () => { + // await doTest(this); + // }); + // } + // await doTest(this); + // } } + function generateRandomString(length: number): string { + const characters = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789"; + let result = ""; + const charactersLength = characters.length; - // async run(): Promise { - // async function doTest(tb: TestBuilder) { - // if (tb.promise.type === "err") { - // const err = tb.promise.val; - // if (tb.expected_error === undefined) throw err; - // if (tb.expected_error === true) return undefined; - // if (tb.expected_error === false) expect(err).toBeUndefined(); - // if (typeof tb.expected_error === "string") { - // expect(err.message).toEqual(tb.expected_error); - // } - // return undefined; - // } - - // const output = await tb.promise.val; - - // const { stdout, stderr, exitCode } = output!; - // if (tb.expected_stdout !== undefined) expect(stdout.toString()).toEqual(tb.expected_stdout); - // if (tb.expected_stderr !== undefined) expect(stderr.toString()).toEqual(tb.expected_stderr); - // if (tb.expected_exit_code !== undefined) expect(exitCode).toEqual(tb.expected_exit_code); - - // for (const [filename, expected] of Object.entries(tb.file_equals)) { - // const actual = await Bun.file(filename).text(); - // expect(actual).toEqual(expected); - // } - // return output; - // } - - // if (this._testName !== undefined) { - // test(this._testName, async () => { - // await doTest(this); - // }); - // } - // await doTest(this); - // } -} -function generateRandomString(length: number): string { - const characters = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789"; - let result = ""; - const charactersLength = characters.length; + for (let i = 0; i < length; i++) { + result += characters.charAt(Math.floor(Math.random() * charactersLength)); + } - for (let i = 0; i < length; i++) { - result += characters.charAt(Math.floor(Math.random() * charactersLength)); + return result; } - return result; + return TestBuilder; } diff --git a/test/js/bun/shell/util.ts b/test/js/bun/shell/util.ts index cafe83ce04fb6..08a632d9b2890 100644 --- a/test/js/bun/shell/util.ts +++ b/test/js/bun/shell/util.ts @@ -4,9 +4,9 @@ import { ShellPromise } from "bun"; import { tempDirWithFiles } from "harness"; import { join } from "node:path"; import * as fs from "node:fs"; -import { TestBuilder } from "./test_builder"; +import { createTestBuilder } from "./test_builder"; -export { TestBuilder }; +export { createTestBuilder }; declare module "bun" { // Define the additional methods From 2ae48f33141727cb0b0134c753881e733147fdae Mon Sep 17 00:00:00 2001 From: Meghan Denny Date: Tue, 16 Apr 2024 14:37:09 -0700 Subject: [PATCH 6/8] lint: ban comparing against undefined in Zig (#10288) --- packages/bun-internal-test/src/banned.json | 4 ++++ src/bun.zig | 10 ++++++++++ src/install/semver.zig | 18 +----------------- 3 files changed, 15 insertions(+), 17 deletions(-) diff --git a/packages/bun-internal-test/src/banned.json b/packages/bun-internal-test/src/banned.json index d3df1c1a0772b..92f96ecfbdd6f 100644 --- a/packages/bun-internal-test/src/banned.json +++ b/packages/bun-internal-test/src/banned.json @@ -3,5 +3,9 @@ "@import(\"root\").bun.": "Only import 'bun' once", "std.mem.indexOfAny": "Use bun.strings.indexAny or bun.strings.indexAnyComptime", "std.debug.print": "Don't let this be committed", + " == undefined": "This is by definition Undefined Behavior.", + " != undefined": "This is by definition Undefined Behavior.", + "undefined == ": "This is by definition Undefined Behavior.", + "undefined != ": "This is by definition Undefined Behavior.", "": "" } diff --git a/src/bun.zig b/src/bun.zig index 100f524bafd1b..024fdfaaee95a 100644 --- a/src/bun.zig +++ b/src/bun.zig @@ -3101,6 +3101,16 @@ pub fn assert(value: bool) callconv(callconv_inline) void { } } +/// This has no effect on the real code but capturing 'a' and 'b' into parameters makes assertion failures much easier inspect in a debugger. +pub inline fn assert_eql(a: anytype, b: anytype) void { + return assert(a == b); +} + +/// This has no effect on the real code but capturing 'a' and 'b' into parameters makes assertion failures much easier inspect in a debugger. +pub inline fn assert_neql(a: anytype, b: anytype) void { + return assert(a != b); +} + pub inline fn unsafeAssert(condition: bool) void { if (!condition) { unreachable; diff --git a/src/install/semver.zig b/src/install/semver.zig index 84562dc2ea67a..fb90aa7815919 100644 --- a/src/install/semver.zig +++ b/src/install/semver.zig @@ -47,22 +47,6 @@ pub const String = extern struct { }; } - pub inline fn init( - buf: string, - in: string, - ) String { - if (comptime Environment.isDebug) { - const out = realInit(buf, in); - if (!out.isInline()) { - assert(@as(u64, @bitCast(out.slice(buf)[0..8].*)) != undefined); - } - - return out; - } else { - return realInit(buf, in); - } - } - pub const Formatter = struct { str: *const String, buf: string, @@ -150,7 +134,7 @@ pub const String = extern struct { } }; - fn realInit( + pub fn init( buf: string, in: string, ) String { From df190815dfc684560cc232457f8d986e83f85d2d Mon Sep 17 00:00:00 2001 From: Meghan Denny Date: Tue, 16 Apr 2024 15:42:24 -0700 Subject: [PATCH 7/8] node: convert remaining js packages to ts (#10289) --- src/js/node/{assert.js => assert.ts} | 2 +- src/js/node/{child_process.js => child_process.ts} | 0 src/js/node/{crypto.js => crypto.ts} | 2 +- .../node/{diagnostics_channel.js => diagnostics_channel.ts} | 0 src/js/node/{dns.js => dns.ts} | 0 src/js/node/{events.js => events.ts} | 0 src/js/node/{fs.js => fs.ts} | 0 src/js/node/{net.js => net.ts} | 0 src/js/node/{punycode.js => punycode.ts} | 0 src/js/node/{querystring.js => querystring.ts} | 4 +++- src/js/node/{readline.js => readline.ts} | 0 src/js/node/{stream.consumers.js => stream.consumers.ts} | 0 src/js/node/{stream.js => stream.ts} | 2 +- src/js/node/{stream.web.js => stream.web.ts} | 0 src/js/node/{timers.promises.js => timers.promises.ts} | 0 src/js/node/{timers.js => timers.ts} | 0 src/js/node/{tls.js => tls.ts} | 0 src/js/node/{url.js => url.ts} | 0 src/js/node/{util.js => util.ts} | 0 src/js/node/{wasi.js => wasi.ts} | 2 +- src/js/node/{zlib.js => zlib.ts} | 2 +- 21 files changed, 8 insertions(+), 6 deletions(-) rename src/js/node/{assert.js => assert.ts} (99%) rename src/js/node/{child_process.js => child_process.ts} (100%) rename src/js/node/{crypto.js => crypto.ts} (99%) rename src/js/node/{diagnostics_channel.js => diagnostics_channel.ts} (100%) rename src/js/node/{dns.js => dns.ts} (100%) rename src/js/node/{events.js => events.ts} (100%) rename src/js/node/{fs.js => fs.ts} (100%) rename src/js/node/{net.js => net.ts} (100%) rename src/js/node/{punycode.js => punycode.ts} (100%) rename src/js/node/{querystring.js => querystring.ts} (98%) rename src/js/node/{readline.js => readline.ts} (100%) rename src/js/node/{stream.consumers.js => stream.consumers.ts} (100%) rename src/js/node/{stream.js => stream.ts} (99%) rename src/js/node/{stream.web.js => stream.web.ts} (100%) rename src/js/node/{timers.promises.js => timers.promises.ts} (100%) rename src/js/node/{timers.js => timers.ts} (100%) rename src/js/node/{tls.js => tls.ts} (100%) rename src/js/node/{url.js => url.ts} (100%) rename src/js/node/{util.js => util.ts} (100%) rename src/js/node/{wasi.js => wasi.ts} (99%) rename src/js/node/{zlib.js => zlib.ts} (99%) diff --git a/src/js/node/assert.js b/src/js/node/assert.ts similarity index 99% rename from src/js/node/assert.js rename to src/js/node/assert.ts index 33f58a9e50d28..f2445c83da7bb 100644 --- a/src/js/node/assert.js +++ b/src/js/node/assert.ts @@ -2,7 +2,7 @@ const util = require("node:util"); var isDeepEqual = Bun.deepEquals; -var __commonJS = (cb, mod) => +var __commonJS = (cb, mod: typeof module | undefined = undefined) => function () { return mod || (0, cb[Object.keys(cb)[0]])((mod = { exports: {} }).exports, mod), mod.exports; }; diff --git a/src/js/node/child_process.js b/src/js/node/child_process.ts similarity index 100% rename from src/js/node/child_process.js rename to src/js/node/child_process.ts diff --git a/src/js/node/crypto.js b/src/js/node/crypto.ts similarity index 99% rename from src/js/node/crypto.js rename to src/js/node/crypto.ts index 2009b0a4ae06d..83e0355a2599a 100644 --- a/src/js/node/crypto.js +++ b/src/js/node/crypto.ts @@ -78,7 +78,7 @@ function getArrayBufferOrView(buffer, name, encoding) { const crypto = globalThis.crypto; const globalCrypto = crypto; -var __commonJS = (cb, mod) => +var __commonJS = (cb, mod: typeof module | undefined = undefined) => function () { return mod || (0, cb[__getOwnPropNames(cb)[0]])((mod = { exports: {} }).exports, mod), mod.exports; }; diff --git a/src/js/node/diagnostics_channel.js b/src/js/node/diagnostics_channel.ts similarity index 100% rename from src/js/node/diagnostics_channel.js rename to src/js/node/diagnostics_channel.ts diff --git a/src/js/node/dns.js b/src/js/node/dns.ts similarity index 100% rename from src/js/node/dns.js rename to src/js/node/dns.ts diff --git a/src/js/node/events.js b/src/js/node/events.ts similarity index 100% rename from src/js/node/events.js rename to src/js/node/events.ts diff --git a/src/js/node/fs.js b/src/js/node/fs.ts similarity index 100% rename from src/js/node/fs.js rename to src/js/node/fs.ts diff --git a/src/js/node/net.js b/src/js/node/net.ts similarity index 100% rename from src/js/node/net.js rename to src/js/node/net.ts diff --git a/src/js/node/punycode.js b/src/js/node/punycode.ts similarity index 100% rename from src/js/node/punycode.js rename to src/js/node/punycode.ts diff --git a/src/js/node/querystring.js b/src/js/node/querystring.ts similarity index 98% rename from src/js/node/querystring.js rename to src/js/node/querystring.ts index d7104cfcda659..73a9a0ac15a72 100644 --- a/src/js/node/querystring.js +++ b/src/js/node/querystring.ts @@ -1,4 +1,6 @@ -var __commonJS = (cb, mod) => () => (mod || cb((mod = { exports: {} }).exports, mod), mod.exports); +var __commonJS = + (cb, mod: typeof module | undefined = undefined) => + () => (mod || cb((mod = { exports: {} }).exports, mod), mod.exports); var Buffer = require("node:buffer").Buffer; diff --git a/src/js/node/readline.js b/src/js/node/readline.ts similarity index 100% rename from src/js/node/readline.js rename to src/js/node/readline.ts diff --git a/src/js/node/stream.consumers.js b/src/js/node/stream.consumers.ts similarity index 100% rename from src/js/node/stream.consumers.js rename to src/js/node/stream.consumers.ts diff --git a/src/js/node/stream.js b/src/js/node/stream.ts similarity index 99% rename from src/js/node/stream.js rename to src/js/node/stream.ts index 245b77eac2a03..58967763c6ac3 100644 --- a/src/js/node/stream.js +++ b/src/js/node/stream.ts @@ -16,7 +16,7 @@ const { var __getOwnPropNames = Object.getOwnPropertyNames; -var __commonJS = (cb, mod) => +var __commonJS = (cb, mod: typeof module | undefined = undefined) => function __require2() { return mod || (0, cb[__getOwnPropNames(cb)[0]])((mod = { exports: {} }).exports, mod), mod.exports; }; diff --git a/src/js/node/stream.web.js b/src/js/node/stream.web.ts similarity index 100% rename from src/js/node/stream.web.js rename to src/js/node/stream.web.ts diff --git a/src/js/node/timers.promises.js b/src/js/node/timers.promises.ts similarity index 100% rename from src/js/node/timers.promises.js rename to src/js/node/timers.promises.ts diff --git a/src/js/node/timers.js b/src/js/node/timers.ts similarity index 100% rename from src/js/node/timers.js rename to src/js/node/timers.ts diff --git a/src/js/node/tls.js b/src/js/node/tls.ts similarity index 100% rename from src/js/node/tls.js rename to src/js/node/tls.ts diff --git a/src/js/node/url.js b/src/js/node/url.ts similarity index 100% rename from src/js/node/url.js rename to src/js/node/url.ts diff --git a/src/js/node/util.js b/src/js/node/util.ts similarity index 100% rename from src/js/node/util.js rename to src/js/node/util.ts diff --git a/src/js/node/wasi.js b/src/js/node/wasi.ts similarity index 99% rename from src/js/node/wasi.js rename to src/js/node/wasi.ts index 53a7448333757..23c8bfac01375 100644 --- a/src/js/node/wasi.js +++ b/src/js/node/wasi.ts @@ -10,7 +10,7 @@ const nodeFsConstants = $processBindingConstants.fs; var __getOwnPropNames = Object.getOwnPropertyNames; -var __commonJS = (cb, mod) => +var __commonJS = (cb, mod: typeof module | undefined = undefined) => function __require2() { return mod || (0, cb[__getOwnPropNames(cb)[0]])((mod = { exports: {} }).exports, mod), mod.exports; }; diff --git a/src/js/node/zlib.js b/src/js/node/zlib.ts similarity index 99% rename from src/js/node/zlib.js rename to src/js/node/zlib.ts index de1260d0ed575..938ad6b82a7c6 100644 --- a/src/js/node/zlib.js +++ b/src/js/node/zlib.ts @@ -10,7 +10,7 @@ const Util = require("node:util"); const { isAnyArrayBuffer, isArrayBufferView } = require("node:util/types"); var __getOwnPropNames = Object.getOwnPropertyNames; -var __commonJS = (cb, mod) => +var __commonJS = (cb, mod: typeof module | undefined = undefined) => function __require() { return mod || (0, cb[__getOwnPropNames(cb)[0]])((mod = { exports: {} }).exports, mod), mod.exports; }; From 3df202f91f8bf315caa1b147c8a94e5a4f79176f Mon Sep 17 00:00:00 2001 From: Stefano Date: Wed, 17 Apr 2024 00:43:47 +0200 Subject: [PATCH 8/8] Fix(windows) search correct path for esbuild(.exe|.cmd) (#10302) --- scripts/make-old-js.ps1 | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/scripts/make-old-js.ps1 b/scripts/make-old-js.ps1 index 42cdf20c85242..b59d95f8675a0 100644 --- a/scripts/make-old-js.ps1 +++ b/scripts/make-old-js.ps1 @@ -3,7 +3,13 @@ $npm_client = "npm" # & ${npm_client} i $root = Join-Path (Split-Path -Path $MyInvocation.MyCommand.Definition -Parent) "..\" + +# search for .cmd or .exe $esbuild = Join-Path $root "node_modules\.bin\esbuild.cmd" +if (!(Test-Path $esbuild)) { + $esbuild = Join-Path $root "node_modules\.bin\esbuild.exe" +} + $env:NODE_ENV = "production"