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

Feature: configurable Nim upstream repo #76

Merged
merged 8 commits into from
Oct 30, 2024

Conversation

gmega
Copy link
Member

@gmega gmega commented Jan 22, 2024

Hi guys, not sure if this is a feature that interests anyone else but, since I needed it and implemented it, I've decided to put out a PR here.

It adds a way to specify a different upstream repo for the Nim compiler (i.e. you can set NIM_REPO=https://github.com/my-org/Nim), in case you want to build it from a fork. Feel free to kill it if not interesting, I'm otherwise happy to fix it if it has issues.

@gmega gmega force-pushed the feat/configurable-nim-repo branch from 2c4bccf to b6a1ad8 Compare January 22, 2024 16:06
@gmega gmega force-pushed the feat/configurable-nim-repo branch from b6a1ad8 to fbce6ef Compare January 22, 2024 16:08
@@ -64,7 +66,8 @@ nim_needs_rebuilding() {
if ! git checkout -q ${NIM_COMMIT} 2>/dev/null; then
# Pay the price for a non-default NIM_COMMIT here, by fetching everything.
# (This includes upstream branches and tags that might be missing from our fork.)
git remote add upstream https://github.com/nim-lang/Nim
git remote remove upstream 2>/dev/null || true
git remote add upstream "${NIM_COMMIT_REPO_URL}"
Copy link
Member

Choose a reason for hiding this comment

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

let's add a new remote name for this, if it is set - ie extra or something like

@gmega gmega force-pushed the feat/configurable-nim-repo branch from 93d92d5 to a44352d Compare June 12, 2024 21:21
@gmega gmega force-pushed the feat/configurable-nim-repo branch from a44352d to 8ef5524 Compare June 12, 2024 21:23
# upstream already exists, call it "extra"
NEW_REMOTE="extra"
fi
git remote add "${NEW_REMOTE}" "${NIM_COMMIT_REPO_URL}"
Copy link
Member

Choose a reason for hiding this comment

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

what happens here if I change the NIM_COMMIT_REPO to a third url? ie I first set it to A then to B?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, sorry, this was not ready yet. :-) We do need to delete the extra if it's already set, precisely for the reason you're pointing out. Will wrap this up and ping.

Copy link
Member Author

@gmega gmega Jun 13, 2024

Choose a reason for hiding this comment

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

OK, I added the delete and did a bit of testing - seems to work as expected. The only potential weirdness I see is that if upstream was not set for some reason and the user does something along the lines of what you said; e.g. sets NIM_COMMIT_REPO to A, then to B, then to C, you'll end up with A being set as upstream (once), then B set to extra, then C set to extra, with A sort of perpetually lingering around as upstream which doesn't look very good.

With that in mind, I'm thinking it would probably make more sense, in the event of a custom commit, to:

  • set upstream always to https://github.com/nim-lang/Nim/ if unset (as before);
  • set whatever is supplied in NIM_COMMIT_REPO, if anything, to extra, replacing what's in there if already set.

Gonna modify it to behave this way - if you think it's a bad idea I can always change it back.

Copy link
Member Author

@gmega gmega Jun 13, 2024

Choose a reason for hiding this comment

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

OK, that's what I have for now (pushed).

@gmega gmega requested a review from arnetheduck June 13, 2024 17:23
@arnetheduck arnetheduck merged commit 0dfdddb into status-im:master Oct 30, 2024
15 checks passed
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.

2 participants