Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support reading from .npmrc #11979

Merged
merged 24 commits into from
Jun 30, 2024
Merged

Support reading from .npmrc #11979

merged 24 commits into from
Jun 30, 2024

Conversation

zackradisic
Copy link
Contributor

What does this PR do?

WIP

This PR implements a .npmrc parser and loads it with Bun's package manager

Fixes #643

Copy link
Contributor

github-actions bot commented Jun 19, 2024

@Jarred-Sumner, your commit has failing tests :(

🪟💻 3 failing tests Windows x64 baseline

  • test/cli/install/registry/bun-install-registry.test.ts 1 failing
  • test/cli/install/registry/bun-install-windowsshim.test.ts code 1
  • test/js/node/child_process/child_process.test.ts 1 failing

🪟💻 3 failing tests Windows x64

  • test/cli/install/registry/bun-install-registry.test.ts 1 failing
  • test/cli/install/registry/bun-install-windowsshim.test.ts code 1
  • test/js/node/child_process/child_process.test.ts 1 failing

View logs

src/ini.zig Outdated Show resolved Hide resolved
@zackradisic zackradisic marked this pull request as ready for review June 24, 2024 21:57
@zackradisic zackradisic marked this pull request as draft June 24, 2024 21:57
@zackradisic zackradisic marked this pull request as ready for review June 26, 2024 02:18
src/ini.zig Outdated Show resolved Hide resolved
src/ini.zig Outdated Show resolved Hide resolved
src/ini.zig Outdated Show resolved Hide resolved
src/ini.zig Outdated Show resolved Hide resolved
src/ini.zig Outdated Show resolved Hide resolved
src/ini.zig Outdated Show resolved Hide resolved
src/ini.zig Outdated Show resolved Hide resolved
src/ini.zig Outdated Show resolved Hide resolved
src/ini.zig Outdated Show resolved Hide resolved
@robobun

This comment was marked as outdated.

src/js_ast.zig Outdated Show resolved Hide resolved
@@ -37,6 +38,7 @@ extern "C" JSPropertyIterator* Bun__JSPropertyIterator__create(JSC::JSGlobalObje
JSC::VM& vm = globalObject->vm();
JSC::JSValue value = JSValue::decode(encodedValue);
JSC::JSObject* object = value.getObject();
ASSERT(object != NULL);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This assertion doesn't do anything, getObject() will crash before this line is reached if that assertion wasn't true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The implementation of getObject() is this which returns a null pointer

inline JSObject* JSValue::getObject() const
{
    return isCell() ? asCell()->getObject() : nullptr;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

isCell() when given a null pointer returns true (yes, surprising)

asCell()->getObject() then de-references the null pointer and 💥

return error.ParserError;
}

// if (!bun.Environment.isDebug) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a reason this is not deleted?

}

const had_errors = log.hasErrors();
log.printForLogLevel(Output.errorWriter()) catch bun.outOfMemory();
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we printing here instead of in the caller?

});
}

// await makeTest([["_authToken", "skibidi"]], result => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this commented out? Should it be deleted or should it be uncommented?

Global.exit(1);
},
}
const source = bun.logger.Source.initPathString(".npmrc", npmrc_contents.items[0..]);
Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner Jun 28, 2024

Choose a reason for hiding this comment

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

Is there a reason we have another implementation of reading a file from disk, instead of using bun.sys.File.toSource or bun.sys.File.toSourceAt?

};
}
}
return .none;
Copy link
Collaborator

Choose a reason for hiding this comment

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

the only return thats necessary is the last one right?

@Jarred-Sumner
Copy link
Collaborator

@zackradisic looks like the test failures in bun-install-registry are real

@Jarred-Sumner Jarred-Sumner merged commit 861be55 into main Jun 30, 2024
51 of 53 checks passed
@Jarred-Sumner Jarred-Sumner deleted the zack/npmrc branch June 30, 2024 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support reading from .npmrc
5 participants