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

CIs: attempt to use csources_v1 #16282

Merged
merged 10 commits into from
Apr 21, 2021
Merged

CIs: attempt to use csources_v1 #16282

merged 10 commits into from
Apr 21, 2021

Conversation

Araq
Copy link
Member

@Araq Araq commented Dec 7, 2020

No description provided.

@Araq Araq requested a review from alaviss December 7, 2020 12:06
Copy link
Collaborator

@alaviss alaviss left a comment

Choose a reason for hiding this comment

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

I don't think we're still using the ci/ scripts.

For ci_docs you'd need to prepend csources_v1- to the cache key so that we have a fully separated cache (not too important as I don't think the commit hash can clash).

You may also want to update azure-pipelines.yml, which is the main CI pipeline.

@@ -15,7 +15,7 @@ environment:
tasks:
- setup: |
cd Nim
git clone --depth 1 -q https://github.com/nim-lang/csources.git
git clone --depth 1 -q https://github.com/nim-lang/csources_v1.git csources

This comment was marked as outdated.

@timotheecour
Copy link
Member

timotheecour commented Dec 8, 2020

since we're for some reason not using git submodules, we need to at least cross reference the git hashes used.

  • please use this instead (I can also volunteer to do it if needed):
export NIM_CSOURCES_HASH=v1.0.0
# or if you don't want to tag csources_v1 yet:
# export NIM_CSOURCES_HASH=2fbbfb191dffdfe0f62460a6aad93f7a1ccd2f5c
  • ditto for windows if windows needs different syntax, under $nim/nimdeps.bat
  • refer to it in each CI pipeline/other bash scripts/build_all.sh/build_all.bat/koch.nim:
source nimdeps.sh
git clone --depth 1 -q https://github.com/nim-lang/csources_v1.git
git -C csources_v1 checkout $NIM_CSOURCES_HASH

Let's not repeat the same mistakes again, we need $NIM_CSOURCES_HASH checked in nim's repo to allow tooling to automatically know which csources to build from in any given revision (including in PR's / branches), eg for git bisect or nim-digger (timotheecour#332)

  • which git hash for nim repo was used to build https://github.com/nim-lang/csources_v1 ? again, this should be explicitly referred to via something like $csources_v1/csourcesdeps.sh pointing to that nim repo hash, analog to $nim/nimdeps.sh

note

  • https://github.com/nim-lang/csources_v1.git instead of https://github.com/nim-lang/csources_v1.git csources is intentional, to avoid conflating with csources and to name a git repo after its clone, as usually done.
    Another benefit is with this, sh build_all.sh + friends won't interfere with csources from before this PR, since it'll instead use csources_v1

@timotheecour timotheecour mentioned this pull request Dec 8, 2020
2 tasks
@alaviss
Copy link
Collaborator

alaviss commented Dec 8, 2020

The idea isn't terrible but I wonder if a shell script is a good idea. Maybe a generic format like csources.txt containing the ref or just a file with a list of <VAR> = <VALUE> since those can be easily parsed without any tooling whatsoever.

@Araq
Copy link
Member Author

Araq commented Dec 8, 2020

Let's not repeat the same mistakes again, we need $NIM_CSOURCES_HASH checked in nim's repo to allow tooling to automatically know which csources to build from in any given revision (including in PR's / branches), eg for git bisect

We don't repeat mistakes, you repeat wrong claims. "git bisect" doesn't need this. There is only one nim binary coming from csources and that is always the start of the bootstrapping process. 1.6 might use the csources_v1 Nim binary.

@@ -9,7 +9,7 @@

## Dead code elimination (=DCE) for IC.

import std / [intsets, tables]
import intsets, tables
Copy link
Member

@timotheecour timotheecour Apr 20, 2021

Choose a reason for hiding this comment

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

why? compiler/ic/dce.nim and intsets are not in the same nimble package

likewise everywhere inside compiler/

Copy link
Member Author

Choose a reason for hiding this comment

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

Bootstrapping on the BSDs doesn't work otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we need csources_v1_2 instead but then I need to go through all the testing trouble again.

Copy link
Member

Choose a reason for hiding this comment

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

Bootstrapping on the BSDs doesn't work otherwise.

is a --lib:lib missing from compilation command in bootstrap? this should be fixable, at least i don't see why it couldn't work; i can take a look if needed;

Maybe we need csources_v1_2 instead

I'm still strongly arguing in favor of latest stable release (1.4.4), (see sample reasons in timotheecour#251) and I'm ready to help if that's what it takes to make it happen. But 1_2 is still better than 1_0 at least.

This PR has tons of unrelated changes to the csources_v1 topic, can't it be 2 PRs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Every change is related to bootstrapping on the BSDs. I presume there is a regression for the 1.0 line where it's pickier about 'std' than even 0.9.x is.

Copy link
Member Author

@Araq Araq Apr 20, 2021

Choose a reason for hiding this comment

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

Also, Nim v1.0 can compile 1.2, 1.4, etc, whereas Nim 1.4 cannot even compile Nim 1.2, last time I checked. We need a csources that can continue to compile the 1.0.x and 1.2.x lines which are still supported and receive backports on a regular basis.

Copy link
Member

@timotheecour timotheecour Apr 20, 2021

Choose a reason for hiding this comment

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

Also, Nim v1.0 can compile 1.2, 1.4, etc

at the price of contorsions like the above comment and many others that prevent code simplifications or using newer features that aid in development of new features/bug fixes;

many times the obvious way to write things didn't work because of boostrap. All that's needed is that each version of nim can be bootstrapped from a deterministic csources version (determined by a stored hash, see #16282 (comment))

Copy link
Member

Choose a reason for hiding this comment

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

Every change is related to bootstrapping on the BSDs. I presume there is a regression for the 1.0 line where it's pickier about 'std' than even 0.9.x is.

see #17801, how do i make bsd CI run in that PR?

Copy link
Member

@timotheecour timotheecour Apr 30, 2021

Choose a reason for hiding this comment

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

indeed, as I suspected, this is fixable simply by passing --lib:lib during bootstrap, see
#17902 for the fix + explanation

how do i make bsd CI run in that PR?

to run bsd CI, the PR I send must originate from nim-lang org, not my own fork; then it works as shown in #17902

@timotheecour
Copy link
Member

timotheecour commented Apr 20, 2021

We don't repeat mistakes, you repeat wrong claims. "git bisect" doesn't need this. There is only one nim binary coming from csources and that is always the start of the bootstrapping process. 1.6 might use the csources_v1 Nim binary.

git bisect needs this.

here's how i run git bisect when i encounter a regression (say at HEAD) that originates beyond last csources update:

  • i first try a few releases to narrow down which is the last release that worked, say v0.18.0:
    git bisect start HEAD v0.18.0
  • then i run:
    git bisect run bash -c "update_csources && bin/nim_csources c -f --skipusercfg --hints:off -o:bin/nim.bisect compiler/nim && bin/nim.bisect -v && bin/nim.bisect c -r --skipparentcfg --skipusercfg -f /pathto/regression.nim"

where update_csources updates bin/nim_csources to a version that can build nim at specified revision; this currently is hard/impossible to do so i have to resort to hacks (eg manual intervention or heuristics based on hardcoded commits).

future proof fix:

With the simple recommendation i gave, update_csources is easy to write, efficient (milliseconds, because of caching bin/nim_csources_144 etc) and 100% automatable:

we store in nim repo in a text file (accessible by bash/nim scripts) the url, commit, or tag corresponding to the csources2 revision that can bootstrap current nim.

with this, the above command in update_csources can be a trivial nim script that checks whether this new file exists, if so gets the revision, builds csources at that revision if not already built, and bootstraps.

It's efficient (each unique bin/nim_csources only needs to be built once even, even across git bisect runs, they're cached), and update_csources simply picks the correct cached bin/nim_csources version.

I can volunteer a PR to write that nim script (tools/nimbisect.nim) if the fix i recommended (simply storing the csources2 hash) is accepted.

@Araq
Copy link
Member Author

Araq commented Apr 20, 2021

here's how i run git bisect when i encounter a regression

why though... who cares if something worked with 0.20, it's archaic and effectively every line of code has been touched since then. I simply look into fixing the bug instead.

@timotheecour
Copy link
Member

timotheecour commented Apr 20, 2021

why though... who cares if something worked with 0.20, it's archaic and effectively every line of code has been touched since then. I simply look into fixing the bug instead.

it doesn't matter much that every LOC has changed, once git bisect returns offending commit it usually becomes clear what the problem is and how to fix the regression, you've narrowed down the search considerably; i and probably others use it frequently.

That's true even if last working version was old, eg see #13066 (from last year, but where regression was introduced before 0.20 and current nim bootstrap doesn't work). without git bisect, it would've been harder to fix the bug (see PR #13303)

For easy bug fixes (where cause and effect are close) it doesn't matter either way, but for harder ones, tooling helps.

note that there are other use cases for ability to build nim at arbitrary past revisions (eg running performance benchmark across nim versions to monitor performance across versions)

And finally, this is future proof; once csources is upgraded, the new one will be less archaic and more likely to be more recent than the point where a regression was introduced.

@Araq
Copy link
Member Author

Araq commented Apr 21, 2021

Merging this now as most changes are preferable anyway. But we can try your suggestions in follow-up PRs.

@Araq Araq merged commit a9b62de into devel Apr 21, 2021
@Araq Araq deleted the araq-csources-update branch April 21, 2021 05:41
@timotheecour
Copy link
Member

But we can try your suggestions in follow-up PRs.

=> #17815

narimiran pushed a commit that referenced this pull request Apr 27, 2021
* CIs: attempt to use csources_v1
* also updated the BSDs
* also updated azure pipelines
* std modules should not itself use the 'std/' import dir...
* compiler has to be careful with std/ for v1 booting

(cherry picked from commit a9b62de)
narimiran pushed a commit that referenced this pull request Apr 27, 2021
* CIs: attempt to use csources_v1
* also updated the BSDs
* also updated azure pipelines
* std modules should not itself use the 'std/' import dir...
* compiler has to be careful with std/ for v1 booting

(cherry picked from commit a9b62de)
narimiran pushed a commit that referenced this pull request Apr 27, 2021
* CIs: attempt to use csources_v1
* also updated the BSDs
* also updated azure pipelines
* std modules should not itself use the 'std/' import dir...
* compiler has to be careful with std/ for v1 booting

(cherry picked from commit a9b62de)
narimiran pushed a commit that referenced this pull request Apr 28, 2021
* CIs: attempt to use csources_v1
* also updated the BSDs
* also updated azure pipelines
* std modules should not itself use the 'std/' import dir...
* compiler has to be careful with std/ for v1 booting

(cherry picked from commit a9b62de)
narimiran pushed a commit that referenced this pull request Apr 28, 2021
* CIs: attempt to use csources_v1
* also updated the BSDs
* also updated azure pipelines
* std modules should not itself use the 'std/' import dir...
* compiler has to be careful with std/ for v1 booting

(cherry picked from commit a9b62de)
narimiran pushed a commit that referenced this pull request Apr 28, 2021
* CIs: attempt to use csources_v1
* also updated the BSDs
* also updated azure pipelines
* std modules should not itself use the 'std/' import dir...
* compiler has to be careful with std/ for v1 booting

(cherry picked from commit a9b62de)
timotheecour added a commit that referenced this pull request Apr 30, 2021
timotheecour added a commit that referenced this pull request Apr 30, 2021
Araq pushed a commit that referenced this pull request Apr 30, 2021
…ap + bsd (#17902)

* [WIP] bring back std/ prefix within compiler and ensure it works in bootstrap + bsd

* refs #16282 (comment)
* sounds very similar to #14291
* more: vmops
* update tools/ci_generate.nim
* auto-generate freebsd.yml as well, to avoid duplication with openbsd.yml
* cleanup
* undo temporary CI removal
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
* CIs: attempt to use csources_v1
* also updated the BSDs
* also updated azure pipelines
* std modules should not itself use the 'std/' import dir...
* compiler has to be careful with std/ for v1 booting
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
…ap + bsd (nim-lang#17902)

* [WIP] bring back std/ prefix within compiler and ensure it works in bootstrap + bsd

* refs nim-lang#16282 (comment)
* sounds very similar to nim-lang#14291
* more: vmops
* update tools/ci_generate.nim
* auto-generate freebsd.yml as well, to avoid duplication with openbsd.yml
* cleanup
* undo temporary CI removal
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.

None yet

3 participants