`choosenim devel --latest` fails build

kaushalmodi commented 3 years ago

Hello,

I saw choosenim devel --build fail on a Travis CI run, and then was also able to reproduce that same issue on Windows 10.

Here's the Travis failure.

... /home/travis/.choosenim/toolchains/nim-#devel/compiler/semtypes.nim(259, 78) Error: undeclared field: 'isNaN' for type system.BiggestFloat [declared in /home/travis/.choosenim/toolchains/nim-#devel/lib/system.nim(1397, 3)] 
... FAILURE

kaushalmodi commented 3 years ago

As @alaviss mentioned here, choosenim needs to start using csources_v1.

/cc @timotheecour

dom96 commented 3 years ago

Why do we have a csources_v1? Any reason we can't replace the C sources choosenim uses with this?

timotheecour commented 3 years ago

the rationale for a new repo csources_v1 instead of csources was avoid breaking scripts from older nim versions relying on sh build_all.sh + friends which were cloning and checking out csources at HEAD; this was a problem because nim's scripts did not check in the version at which to checkout csources, ie it was not forward compatible.

This new design is forward compatible, as it specifies the version (git hash) of csources_v1 from which to bootstrap (analog to a git submodule). All those details are abstracted out via a bash API which is the recommended way to bootstrap from csources_v1, and which all the CI scripts in nim repo now follow:

. ci/funs.sh && nimBuildCsourcesIfNeeded # builds nim from csources_v1
sh build_all.sh # builds nim from csources_v1, koch, nim, tools

choosenim should be updated as follows:

for building nim versions >= 1.5.1, call sh build_all.sh (posix) or build_all.bat (windows) directly, instead of calling most of the code in src/choosenimpkg/builder.nim; this should be forward compatible even when csources_v1 gets updated
for nim version >= 1.4.8 < 1.5.1, it also uses csources_v1 which was backported, possibly using the same approach as for >= 1.5.1 should work (I haven't checked how it was backported)
for building older nim versions, maybe keep the existing logic in src/choosenimpkg/builder.nim if it works; IIUC this requires inferring which csources hash to bootstrap from for a particular nim version (that logic can be reused for a nim git bisect tool analog to D's digger https://github.com/CyberShadow/Digger, refs https://github.com/timotheecour/Nim/issues/332)

If for some reason (please explain) something more granular is needed than build_all, then at least . ci/funs.sh && nimBuildCsourcesIfNeeded should be used to avoid leaking internal implementation details.

dom96 commented 3 years ago

Would it be possible to provide a backward compatible wrapper around this new mechanism? i.e. a build.sh/build.bat/build64.bat that just calls sh build_all.sh/build_all.bat. I'd rather avoid a situation where choosenim needs to do different things based on the version of Nim we get.

timotheecour commented 3 years ago

how would that help given that older nim versions don't have such a $nim/build.sh? (unless you're talking about $nim/ci/build.sh which existed before but are broken and do different things)

the only correct way is to customize the logic based on which nim version is being built; it should be as simple as sh build_all.sh for newer versions going forward, and older versions can reuse old logic if it still works.

I do intend to write a nim bisect tool at some point with the aim of being able to build nim (just nim, no tools) at any prior git hash to help with git bisect workflows, the way it'll work is:

for new nim versions, use . ci/funs.sh && nimBuildCsourcesIfNeeded (which also already handles caching)
for older nim versions relying on csources, figure out which csources hash to checkout based on git fetch --tags and git tag and finding the latest tag, the using a table lookup to identify the appropriate csources hash. Sadly necessary because of lack of prior design but not very complicated (followup here for this: https://github.com/timotheecour/Nim/issues/332)

dom96 commented 3 years ago

I'm talking about the build.sh file that's in the C sources. https://github.com/nim-lang/csources/blob/master/build.sh

I see that the new C sources also have this: https://github.com/nim-lang/csources_v1/blob/master/build.sh. So Im once again confused why we have a new repo for this.

timotheecour commented 3 years ago

So Im once again confused why we have a new repo for this.

as explained above, so that existing scripts calling sh build_all.sh don't break for older nim versions 0.20.0 onwards, eg:

git clone https://github.com/nim-lang/Nim
cd Nim
git checkout v1.2.0 # or >= v0.20.0 (but breaks for < 0.20.0 because of csources HEAD)
sh build_all.sh

this will continue to work in the future

if csources were updated, the above script would break because in older nim versions, sh build_all.sh would pickup csources HEAD instead of a version tied to the nim version being bootstrapped. That previous design was bad because it wasn't forward compatible; the new one is forward compatible as it doesn't blindly build from csources HEAD but instead from a version that's checked in nim repo.

dom96 commented 3 years ago

build_all is new to me, I see it downloads the C sources which is a change from the old bootstrap method (where you were expected to clone HEAD of the C sources repo manually).

How about for simplicity choosenim just detects the existance of the build_all scripts and calls them instead of cloning C sources? Would that work?

Btw as an aside, from https://github.com/nim-lang/csources readme:

This repository is archived because it's frozen, HEAD of csources can build Nim version 1 and any later version.

This clearly says that HEAD of https://github.com/nim-lang/csources should be able to build any Nim version 1.0+. Did we change our mind? If so we should update the readme.

timotheecour commented 3 years ago

How about for simplicity choosenim just detects the existance of the build_all scripts and calls them instead of cloning C sources? Would that work?

as noted in https://github.com/dom96/choosenim/issues/256#issuecomment-848100057, this is indeed what should be done but only to building nim >= 0.20.0, as it'd fail for versions < 0.20.0 even with build_all.sh present (see explanation above).

here's how i'm doing it here: https://github.com/nim-lang/Nim/pull/18119 (for nimdigger tool)

If so we should update the readme.

indeed, this readme should be updated to reflect reality; the promise of allowing it to build any nim version >= 1.0 was a false good idea, for reasons discussed elsewhere (eg https://github.com/timotheecour/Nim/issues/251 + links) ; eg prevents using more recent nim to bootstrap nim (ie from benefitting from bugfixes + features + cleanups in the compiler), or, in this case, from boostrapping from apple M1.

dom96 commented 3 years ago

Building via build_all.sh doesn't work for me:

$ ./build_all.sh
: not found.sh: 3: ./build_all.sh:
: not found.sh: 7: ./build_all.sh:
: not found.sh: 10: ./build_all.sh:

I also tried:

$ . ci/funs.sh && nimBuildCsourcesIfNeeded
: command not found
-bash: ci/funs.sh: line 6: syntax error near unexpected token `$'{\r''
'bash: ci/funs.sh: line 6: `echo_run () {

(Plus sh build_all.sh and bash build_all.sh to no avail)

Any ideas what could be going wrong?

timotheecour commented 3 years ago

can you show commands starting from scratch?

git clone https://github.com/nim-lang/Nim
cd Nim
sh build_all.sh

dom96 commented 3 years ago

Not sure what info that can give you, but sure:

dom@DESKTOP-AAP3EIS:/tmp$ git clone https://github.com/nim-lang/Nim
Cloning into 'Nim'...
remote: Enumerating objects: 143822, done.
remote: Counting objects: 100% (589/589), done.
remote: Compressing objects: 100% (180/180), done.
remote: Total 143822 (delta 429), reused 525 (delta 408), pack-reused 143233
Receiving objects: 100% (143822/143822), 105.01 MiB | 5.83 MiB/s, done.
Resolving deltas: 100% (110719/110719), done.
Checking out files: 100% (2964/2964), done.
dom@DESKTOP-AAP3EIS:/tmp$ cd Nim
dom@DESKTOP-AAP3EIS:/tmp/Nim$ sh build_all.sh
: not foundh: 3: build_all.sh:
: not foundh: 7: build_all.sh:
: not foundh: 10: build_all.sh:

timotheecour commented 3 years ago

what is:

uname -v (at least, which OS is that)
which sh? (+ version)

2 possibilities:

according to https://stackoverflow.com/a/14785093/1426932, \r would produce such output, but od -xcb build_all.sh doesn't show presence of \r ; please double check on your system though (could be that git fetches CRLF according to some policy in gitconfig)
space in shebang (refs https://github.com/nim-lang/Nim/pull/18444); please retry with this PR applied

dom96 commented 3 years ago

$ uname -v
#488-Microsoft Mon Sep 01 13:43:00 PST 2020

(It's Ubuntu 18.04 on WSL)

$ $SHELL --version
GNU bash, version 4.4.20(1)-release (x86_64-pc-linux-gnu)
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

(No idea how to get sh version)

$ which sh
/bin/sh

but yes, it looks like it's problems with CRLF...

$ bash -x build_all.sh
+ $'\r'
build_all.sh: line 3: $'\r': command not found
+ $'\r'
build_all.sh: line 7: $'\r': command not found
+ set -u
+ set -e
+ $'\r'
build_all.sh: line 10: $'\r': command not found

timotheecour commented 3 years ago

(It's Ubuntu 18.04 on WSL)

well that was a pretty big clue...

seems like same problem as described here: https://github.com/microsoft/WSL/issues/2318

try https://github.com/nim-lang/Nim/pull/18452

kaushalmodi commented 3 years ago

@dom96 Does that commit to Nim repo fix this issue? I thought choosenim needed to switch to the build_all.sh script to fix this.

dom96 commented 3 years ago

yeah, it doesn't.

timotheecour commented 3 years ago

yes, I should've phrased https://github.com/nim-lang/Nim/pull/18452 differently, that PR fixed https://github.com/dom96/choosenim/issues/256#issuecomment-874282997 (AFAIU) which is a pre-requisite but choosenim still needs to be updated to call build_all.sh for nim >= 0.20.0, and use old method for nim < 0.20.0 as explained here: https://github.com/dom96/choosenim/issues/256#issuecomment-850714556

dom96 commented 3 years ago

Why >= 0.20.0? AFAIK the old method works quite well even for 1.4 no?

timotheecour commented 3 years ago

Why >= 0.20.0? AFAIK the old method works quite well even for 1.4 no?

not for 1.4.8

my point is you can't use the same method for every release, so you have to build via build_all.sh for any recent enough nim, ie nim >= 1.4.8, refs https://nim-lang.org/blog/2021/05/25/version-148-released.html

Just like our devel branch, v1.4.8 is built using csources_v1, which means you can use it on Apple M1 chips.

and you can't use build_all.sh for nim < 0.20.0 as explained above.

where you put the threshold is up to you, but that seemed like a reasonable option.

dom96 / choosenim

`choosenim devel --latest` fails build #256