Skip to content

bootstrap: Use dependencies.txt to list packages #179

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 3, 2016
Merged

bootstrap: Use dependencies.txt to list packages #179

merged 2 commits into from
Jul 3, 2016

Conversation

petertseng
Copy link
Member

Thanks to #159, we have a list of dependencies of each exercise. We
should replace the current hard-coded list in bootstrap.sh with one
inferred from dependencies.txt.

Closes #175

petertseng
petertseng reviewed Jul 3, 2016
View reviewed changes
_test/bootstrap.sh

if echo "$deps" | grep -q 'old-locale'; then
# Move old-locale to the bottom.
# Unfortunate, but without this cabal-install tries to update old-locale.
Copy link
Member Author

Choose a reason for hiding this comment

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

If you ask me to explain why, I have no idea.

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 I should grab a machine with GHC 7.6 or 7.8 and investigate, because an explanation would be great.

Copy link
Contributor

@rbasso rbasso Jul 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

To compare lists of itens, comm from GNU coreutils is usually a good choice. Try this:

comm -23 <(dependencies `ls exercises`) <(ghc-pkg list --simple-output --names-only | xargs -n1 | sort)

In my local setup I cannot test it easily, but it will probably work.

Edit: Sorry! Now I see you already did that below.

Copy link
Member Author

Choose a reason for hiding this comment

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

although the xargs -n1 is one thing I was missing. I settled on tr " " "\n" instead, guess they do the same thing

@petertseng
Copy link
Member Author

OK, testing a few things on Travis leads me to think this will be OK now.

The duplicate PRs with this one are unfortunate, should test things out on Travis on my fork of the repo rather than here, didn't get that idea until while working on this PR.

@petertseng
Copy link
Member Author

petertseng commented Jul 3, 2016 •
edited
Loading

On reviewing Travis logs:

set -x is pretty noisy. Do we want to keep it?

@rbasso
Copy link
Contributor

rbasso commented Jul 3, 2016

Don't worry about the extra PRs...I did the same a few days ago.
I don't like set -x too much in production, but I don't care so much about it. You can decide depending on how much you trust the code will run correctly.

@petertseng
Copy link
Member Author

I guess my main complaint is that we see the contents of $deps and $already_installed twice each: Once when they get set, and again when they get interpolated in the echo.

We probably don't even need to see deps since that can be reproduced by locally running dependencies. I'm not sure already_installed gets much use.

Based on this, perhaps remove set -x and just print out $not_installed will do.

petertseng
petertseng reviewed Jul 3, 2016
View reviewed changes
_test/bootstrap.sh
already_installed=$(ghc-pkg list --names-only --simple-output | tr " " "\n" | sort)
not_installed=$(comm -23 <(echo "$deps") <(echo "$already_installed"))

echo "Not installed packages ($(echo "$not_installed" | wc -l)):"
Copy link
Member Author

@petertseng petertseng Jul 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

sigh, off by one error. Not installed packages (1): when not_installed == ""

Copy link
Member Author

Choose a reason for hiding this comment

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

echo -n it is.

@rbasso rbasso mentioned this pull request Jul 3, 2016
Closed
petertseng added 2 commits July 3, 2016 03:11
@petertseng
bootstrap: Use dependencies.txt to list packages
d05bc9b 
Thanks to #159, we have a list of dependencies of each exercise. We
should replace the current hard-coded list in `bootstrap.sh` with one
inferred from `dependencies.txt`.

Closes #175
@petertseng
bootstrap: remove set -x in favor of printing not_installed
fda2c78 
too noisy to see `deps` and `already_installed` so many times. It's easy
to locally reproduce the value of `deps` and `already_installed` isn't
so interesting. We'll just print out `not_installed`.
@petertseng
Copy link
Member Author

Heh, echo -n undercounts by 1 when there is a nonempty list of packages to update (tested by killing caches on my fork). Going to have to just do different things based on -z "$not_installed".

I am satisfied with this now.

@petertseng petertseng mentioned this pull request Jul 3, 2016
Merged
@rbasso
Copy link
Contributor

rbasso commented Jul 3, 2016 •
edited
Loading

Ok. I'll clear all the caches and rerun the Travis builds just to be sure everything is fine before merging this one.
Edit: Nothing seems to depend on the old caches. 👍

@rbasso
Copy link
Contributor

rbasso commented Jul 3, 2016

Would you mind if I squash those two commits in a single one?

@petertseng
Copy link
Member Author

OK with me to squash

@rbasso rbasso merged commit cd60652 into exercism:master Jul 3, 2016
@petertseng petertseng deleted the bootstrap-deps branch July 3, 2016 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@petertseng @rbasso