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

choosenim devel --latest fails build #256

Closed
kaushalmodi opened this issue May 25, 2021 · 20 comments · Fixed by nim-lang/Nim#18452
Closed

choosenim devel --latest fails build #256

kaushalmodi opened this issue May 25, 2021 · 20 comments · Fixed by nim-lang/Nim#18452

Comments

@kaushalmodi
Copy link

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
Copy link
Author

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

/cc @timotheecour

@dom96
Copy link
Owner

dom96 commented May 25, 2021

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

@timotheecour
Copy link
Contributor

timotheecour commented May 25, 2021

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 nim-digger: program to build nim at any revision (including forks/PRs) (cf D's digger); useful for git bisect timotheecour/Nim#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
Copy link
Owner

dom96 commented May 25, 2021

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
Copy link
Contributor

timotheecour commented May 25, 2021

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:

@dom96
Copy link
Owner

dom96 commented May 25, 2021

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
Copy link
Contributor

timotheecour commented May 25, 2021

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
Copy link
Owner

dom96 commented May 26, 2021

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
Copy link
Contributor

timotheecour commented May 28, 2021

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 #256 (comment), 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: nim-lang/Nim#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 timotheecour/Nim#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
Copy link
Owner

dom96 commented Jul 5, 2021

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
Copy link
Contributor

can you show commands starting from scratch?

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

@dom96
Copy link
Owner

dom96 commented Jul 5, 2021

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
Copy link
Contributor

timotheecour commented Jul 5, 2021

what is:

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

2 possibilities:

@dom96
Copy link
Owner

dom96 commented Jul 7, 2021

$ 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
Copy link
Contributor

timotheecour commented Jul 7, 2021

(It's Ubuntu 18.04 on WSL)

well that was a pretty big clue...

seems like same problem as described here: microsoft/WSL#2318

try nim-lang/Nim#18452

timotheecour added a commit to timotheecour/Nim that referenced this issue Jul 8, 2021
dom96 pushed a commit to nim-lang/Nim that referenced this issue Jul 14, 2021
@kaushalmodi
Copy link
Author

@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
Copy link
Owner

dom96 commented Jul 14, 2021

yeah, it doesn't.

@dom96 dom96 reopened this Jul 14, 2021
@timotheecour
Copy link
Contributor

timotheecour commented Jul 14, 2021

yes, I should've phrased nim-lang/Nim#18452 differently, that PR fixed #256 (comment) (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: #256 (comment)

@dom96
Copy link
Owner

dom96 commented Jul 14, 2021

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

@timotheecour
Copy link
Contributor

timotheecour commented Jul 14, 2021

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 dom96 closed this as completed in 3963a62 Oct 2, 2021
PMunch pushed a commit to PMunch/Nim that referenced this issue Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants