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

Use full repo when building windows installer #1232

Merged
merged 1 commit into from
May 1, 2021

Conversation

pramodk
Copy link
Member

@pramodk pramodk commented May 1, 2021

  • by default depth is 1 and hence only HEAD is checked out
  • this gives version below i.e. tag is missing
    NEURON -- VERSION + HEAD (0e9a051+) 2021-04-30

 * by default depth is 1 and hence only HEAD is checked out
 * this gives version below i.e. tag is missing
    NEURON -- VERSION + HEAD (0e9a051+) 2021-04-30
@pramodk pramodk requested a review from alexsavulescu May 1, 2021 06:26
@pramodk
Copy link
Member Author

pramodk commented May 1, 2021

Seems like Azure pipelines don't do shallow clone by default but GitHub Actions do:

image

@codecov-commenter
Copy link

Codecov Report

Merging #1232 (8dcc8ee) into master (b159f76) will increase coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1232      +/-   ##
==========================================
+ Coverage   31.87%   31.95%   +0.08%     
==========================================
  Files         572      572              
  Lines      108988   108988              
==========================================
+ Hits        34739    34829      +90     
+ Misses      74249    74159      -90     
Impacted Files Coverage Δ
src/parallel/bbs.cpp 64.78% <0.00%> (+1.87%) ⬆️
src/parallel/bbssrv2mpi.cpp 56.89% <0.00%> (+5.74%) ⬆️
src/nrnmpi/bbsmpipack.cpp 86.44% <0.00%> (+12.42%) ⬆️
src/parallel/bbsclimpi.cpp 50.00% <0.00%> (+20.12%) ⬆️
src/parallel/bbssrvmpi.cpp 43.03% <0.00%> (+29.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b159f76...8dcc8ee. Read the comment docs.

@pramodk pramodk merged commit 5e68f3e into master May 1, 2021
@pramodk pramodk deleted the prmaodk/windows-installer-version branch May 1, 2021 11:27
pramodk added a commit that referenced this pull request May 1, 2021
* by default depth is 1 and hence only HEAD is checked out
* this gives version below i.e. tag is missing
    NEURON -- VERSION + HEAD (0e9a051+) 2021-04-30
@nrnhines
Copy link
Member

nrnhines commented May 1, 2021

Isn't this a sledgehammer for a gnat? What depth would be sufficient to get a good NEURON version?

@pramodk
Copy link
Member Author

pramodk commented May 1, 2021

I think there is no easy way - we need to know the last tag for git describe and that can't be fixed? this is default behaviour on Azure CI but not on GitHub Actions.

alexsavulescu pushed a commit that referenced this pull request May 3, 2021
* by default depth is 1 and hence only HEAD is checked out
* this gives version below i.e. tag is missing
    NEURON -- VERSION + HEAD (0e9a051+) 2021-04-30
@alkino
Copy link
Member

alkino commented Aug 9, 2021

maybe use --single-branch

On my computer:
full: 12min 03sec
depth 500: 18sec
single-branch: 43sec

@pramodk @nrnhines ?

@nrnhines
Copy link
Member

nrnhines commented Aug 9, 2021

--single-branch is definitely a solution to to the git describe problem. And for me it is an order of magnitude faster as well.

hines@Michaels-MacBook-Pro-2 neuron % time git clone http://github.com/neuronsimulator/nrn --branch netcvode-cover --single-branch temp
Cloning into 'temp'...
warning: redirecting to https://github.com/neuronsimulator/nrn/
remote: Enumerating objects: 27039, done.
remote: Counting objects: 100% (21/21), done.
remote: Compressing objects: 100% (12/12), done.
remote: Total 27039 (delta 9), reused 20 (delta 9), pack-reused 27018
Receiving objects: 100% (27039/27039), 36.82 MiB | 3.97 MiB/s, done.
Resolving deltas: 100% (18782/18782), done.
git clone http://github.com/neuronsimulator/nrn --branch netcvode-cover  temp  2.82s user 1.01s system 37% cpu 10.121 total
hines@Michaels-MacBook-Pro-2 neuron % (cd temp ; git describe)
8.0a-488-gc7b37ec2
hines@Michaels-MacBook-Pro-2 neuron % (cd temp ; git status)
On branch netcvode-cover
Your branch is up to date with 'origin/netcvode-cover'.

nothing to commit, working tree clean

@nrnhines
Copy link
Member

nrnhines commented Aug 9, 2021

full: 12min 03sec

The --single-branch solution for CI seems to me to be a good one but is not so great for development involving pull and push. For my local environment which often does not have high bandwidth to GitHub, I've gotten into the habit of
having a local mirror clone, nrngit, which I use only with a newpr.sh script with the substantive contents:

cd $HOME/neuron/nrngit
git pull
git-checkout-all-remote

cd $HOME/neuron
git clone nrngit $1
cd $HOME/neuron/$1
git remote set-url origin [email protected]:neuronsimulator/nrn
git pull

The git-checkout-all-remote script is

#!/bin/sh
git remote prune origin
git branch | grep -v "master" | xargs git branch -D 
git pull --all
git branch -r | grep -v '\->' | while read remote; do git branch --track "${remote#origin/}" "$remote"; done

So now the time it takes work on a fresh clone from GitHub is not onerous, e.g.

time newpr.sh temp
...
0.99s user 0.58s system 39% cpu 3.941 total

The flaw, of course, is that subprocess cloneing time is not reduced when I need CoreNEURON, NMODL, etc.

@alkino
Copy link
Member

alkino commented Aug 9, 2021

My solution is only for CI, nothing more.

On local if you remove all branches it will be hard to work.

Locally you can clone it once, and update the repository each day?

@alexsavulescu
Copy link
Member

Unfortunately this feature has to be supported by GHA/Azure, or at least have some simple script on top of their checkout mechanisms. Otherwise there are several events to take into account (PRs, merge, release branches, manual dispatch, ...) and that would take dedicated time to cover & properly test & more maintenance .

I can maybe propose a compromise: depth=1 whenever we have PRs/merges and depth=0 when we do deliveries(here it's more about the Windows installer and wheels). The downside would be that for depth=1 artifacts we may not know exactly where they originated from, i.e. 5e68f3e

@alexsavulescu alexsavulescu mentioned this pull request Mar 22, 2022
15 tasks
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.

5 participants