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

Fix issues found while trying to run download-env #373

Merged
merged 3 commits into from Nov 5, 2017
Merged

Fix issues found while trying to run download-env #373

merged 3 commits into from Nov 5, 2017

Conversation

tsukasa-au
Copy link
Contributor

Fix some of the issues I found while trying to run the linux image on qemu.

Specifically:

  • Disable warnings as errors when compiling qemu
  • Complain to the user if the path to the repository is known to break (contains either whitespace or ':')
  • Make the download-env script continue to attempt to install conda if a previous attempt failed

@mithro
Copy link
Member

mithro commented Nov 5, 2017

Can you do a git rebase -i timvideos/master; git push tukasa xxx:mystuff-to-push -f

@@ -31,6 +31,20 @@ if [ ! -z "$SETTINGS_FILE" -o ! -z "$XILINX" ]; then
exit 1
fi

# Conda does not support ' ' in the path (it bails early).
if echo "${SETUP_DIR}" | grep -q ' '; then
Copy link
Member

Choose a reason for hiding this comment

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

Can you indent this to match the existing code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

./Miniconda3-latest-Linux-x86_64.sh -p $CONDA_DIR -b
# -p to specify the install location
# -b to enable batch mode (no prompts)
# -f to not return an error if the location specified by -p already exists
Copy link
Member

Choose a reason for hiding this comment

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

Indenting.

cd $BUILD_DIR
wget --no-verbose -c https://repo.continuum.io/miniconda/Miniconda3-latest-Linux-x86_64.sh
wget --no-verbose --continue https://repo.continuum.io/miniconda/Miniconda3-latest-Linux-x86_64.sh
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a # FIXME: Get the miniconda people to add a "self check" mode

echo "You appear to have ':' characters in the path to this script."
echo "Please move this repository to another path that does not contain this character."
exit 1
fi
Copy link
Member

Choose a reason for hiding this comment

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

Could you add these checks to enter-env.sh as well? You should be able to do; vimdiff download-env.sh enter-env.sh and see how they are mostly trying to be the same.

This allows qemu to be build on arch linux, it should not break other
systems where this is the default.
We now ensure that a conda binary actually exists, rather than just the
directory we asked it to be installed in. The install script will now
attempt to install conda on subsequent runs, if a previous install
attempt failed.
Provide the user with a "nice" error message if the repository exists in
a directory that is known to not work (ones with whitespace or with
colon's in the path).
Copy link
Contributor Author

@tsukasa-au tsukasa-au left a comment

Choose a reason for hiding this comment

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

Rebased - removed the linux related commit that I pulled from your repo (you can pull that in yourself if you want it).

@mithro
Copy link
Member

mithro commented Nov 5, 2017

Looks great!

@mithro mithro merged commit 0c06257 into timvideos:master Nov 5, 2017
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.

None yet

2 participants