Skip to content
This repository was archived by the owner on Jun 30, 2025. It is now read-only.

Conversation

@juliangruber
Copy link
Member

@juliangruber juliangruber requested a review from bajtos June 14, 2023 12:38
repo,
distTag,
executable,
arch = os.arch(),
Copy link
Member Author

Choose a reason for hiding this comment

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

this argument wasn't being used any more

Copy link
Member

Choose a reason for hiding this comment

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

I am concerned about setting the arch deep inside this file. When reading the top-level post-install script, it's not clear that we may be downloading different files based on some environment variables.

Would you mind keeping arch here, maybe as a required argument, and setting it from env vars in the file where we are calling getBinaryModuleExecutable?

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea, like this?

repo,
distTag,
executable,
arch = os.arch(),
Copy link
Member

Choose a reason for hiding this comment

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

I am concerned about setting the arch deep inside this file. When reading the top-level post-install script, it's not clear that we may be downloading different files based on some environment variables.

Would you mind keeping arch here, maybe as a required argument, and setting it from env vars in the file where we are calling getBinaryModuleExecutable?

@juliangruber juliangruber requested a review from bajtos June 14, 2023 13:12
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

👏🏻

@juliangruber juliangruber merged commit 9c40a23 into main Jun 14, 2023
@juliangruber juliangruber deleted the add/arch-install branch June 14, 2023 13:16
@juliangruber
Copy link
Member Author

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants