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

refactor all code that builds csources #17815

Merged
merged 17 commits into from
Apr 23, 2021
Merged

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Apr 21, 2021

  • follows CIs: attempt to use csources_v1 #16282, refs CIs: attempt to use csources_v1 #16282 (comment)
  • refactor all code that builds csources so that changing csources url/commit can be done in a single place (nimBuildCsourcesIfNeeded), without affecting scripts
  • updating csources_v1 won't affect nim repo since this PR hardcodes the hash at which to build csources_v1
  • sh build_all.sh now does "the right thing" transparently (caches bin/nim_csources_$hash and only rebuilds when needed)
  • remove ci/* except ci/funs.sh, which by design has no top-level statement, only reusable bash functions that can be used after . ci/funs.sh
  • simplify CI pipelines (as you can see, the diff removes code overall)
  • fixes Lock down csources version #11457

design goals

  • DRY: hide the way we build csources (bootstrap nim) behind a bash function call, nimBuildCsourcesIfNeeded
  • reusable bash functions for CI + build_all (note we still prefer nim scripts but this is for bootstrapping)
  • future proof design so that we can build nim at any revision even if csources url/hash is later updated (ie we tie a nim version to a corresponding csources version), allowing tooling eg:
    • git bisect workflows
    • running benchmarks across several nim versions
  • we won't need a new git repo csources_v2 etc, instead we can reuse csources_v1 with different commits (or tags/branches if needed for clarity)

notes

  • I haven't updated appveyor.yml.disabled, it's outdated and not used anymore
  • I haven't updated build_all.bat, it was already outdated (used csources instead of csources_v1), and I don't know if it's still used by windows users (given we have choosenim and that windows users can use bash scripts IIRC, eg as does azure_pipelines.yml); if it still needs to be maintained, we can update it (or even auto-generate it to keep the urls/hash in sync)
  • I've removed the caching logic in ci_docs.yml because it wasn't used in other pipelines, it doesn't save much CI time, and it simplifies the pipeline quite a bit; if needed i can revert that change

future work

links

@timotheecour timotheecour marked this pull request as ready for review April 22, 2021 04:01
ci/funs.sh Outdated Show resolved Hide resolved
@timotheecour timotheecour force-pushed the pr_refactor_cis branch 3 times, most recently from 62f04a0 to 051ed10 Compare April 22, 2021 19:34
@timotheecour timotheecour added the Ready For Review (please take another look): ready for next review round label Apr 22, 2021
@RSDuck
Copy link
Contributor

RSDuck commented Apr 22, 2021

I haven't updated build_all.bat, it was already outdated (used csources instead of csources_v1), and I don't know if it's still used by windows users (given we have choosenim and that windows users can use bash scripts IIRC, eg as does azure_pipelines.yml); if it still needs to be maintained, we can update it (or even auto-generate it to keep the urls/hash in sync)

I did not too long ago, because it's mentioned readme, so I would assume other people do this as well.

@timotheecour
Copy link
Member Author

well I've updated it in the meantime in a prior commit, please test build_all.bat from this PR

@RSDuck
Copy link
Contributor

RSDuck commented Apr 22, 2021

everything seems to work well. Is there anything in particular I should test?

@timotheecour
Copy link
Member Author

everything seems to work well. Is there anything in particular I should test?

that's it, thanks!

.travis.yml Show resolved Hide resolved
build_all.sh Outdated Show resolved Hide resolved
ci/funs.sh Outdated Show resolved Hide resolved
koch.nim Outdated Show resolved Hide resolved
@timotheecour timotheecour added TODO: followup needed remove tag once fixed or tracked elsewhere and removed Ready For Review (please take another look): ready for next review round labels Apr 23, 2021
@Araq Araq added the merge_when_passes_CI mergeable once green label Apr 23, 2021
@Araq Araq merged commit dce0b3b into nim-lang:devel Apr 23, 2021
@timotheecour timotheecour deleted the pr_refactor_cis branch April 23, 2021 09:35
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
* refactor all code that builds csources
* fixup
* nim_csourcesDir_v0 + nim_csourcesDir
* remove deprecated, unused scripts from ci/
* reuse nimCsourcesHash in ci
* simplify CI pipelines by reusing nimBuildCsourcesIfNeeded
* simplify ci_docs.yml by reusing nimBuildCsourcesIfNeeded
* cleanup
* use csources_v1 as destination dir
* fixup
* remove pushCsources
* address comment: remove build.sh support for now
* fixup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge_when_passes_CI mergeable once green TODO: followup needed remove tag once fixed or tracked elsewhere
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lock down csources version
3 participants