Skip to content

Comments

Check for dub#218

Merged
wilzbach merged 5 commits intodlang:masterfrom
wilzbach:check-for-dub
Dec 22, 2017
Merged

Check for dub#218
wilzbach merged 5 commits intodlang:masterfrom
wilzbach:check-for-dub

Conversation

@wilzbach
Copy link
Contributor

@wilzbach wilzbach commented Mar 2, 2017

Also seems to fail, at least during initial installation, due to return 1 and set -e (errexit).

Can't reproduce this. Added tests for it to let Travis verify this.

Mmh, this still queries code.dlang.org when a compiler is already installed.

Added a check. My main motivation was really just to avoid unnecessary downloads on Travis

You also didn't adopt the activate scripts to work w/o a separate dub installation.
It still "worked", because of the $PATH variable.

Now the dub folders shouldn't been written to the activate scripts anymore.

I'll revert this for now to not impact any usage.

Sorry about that :/

@wilzbach wilzbach force-pushed the check-for-dub branch 2 times, most recently from bddcf93 to ff6119f Compare March 2, 2017 00:37
@MartinNowak MartinNowak self-requested a review June 13, 2017 12:08
@MartinNowak MartinNowak self-assigned this Jun 13, 2017
@MartinNowak MartinNowak removed their request for review June 13, 2017 12:09
@wilzbach
Copy link
Contributor Author

@MartinNowak: can we move forward with this?
Downloading dub from code.dlang.org adds yet another server which is rather unstable to the mix:

https://travis-ci.org/dlang/dmd/jobs/276138166

(especially when it's already existent)

@MetaLang
Copy link
Member

Doesn't this mean that dub could get out of date on the host machine?

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@wilzbach
Copy link
Contributor Author

Doesn't this mean that dub could get out of date on the host machine?

You meant that you have an old D compiler and still want to use the newest DUB? Well, definitely a use case, but downloading a newer DUB yourself isn't that difficult ...
However, note that there's also the contrary point that newer DUB versions might drop (some) support for old compilers.

The point of this issue / PR was that on the CI machines we essentially download DUB twice and in particular the download from code.dlang.org is problematic as code.dlang.org has longer downtimes fairly regularly.

@MartinNowak
Copy link
Member

We could add explicit installs for e.g. dub-1.6.0, would need a separate, nestable activate/deactivate script though.

Copy link
Member

@MartinNowak MartinNowak left a comment

Choose a reason for hiding this comment

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

Looks OKish, but could be done nicer I guess.
How about adding a install() function and call install_compiler and install_dub from there? Also now you construct binpath twice (though one instance is named bin_path).


if [ "$HAS_DUB_INCLUDED" -eq 0 ] ; then
install_dub
fi
Copy link
Member

Choose a reason for hiding this comment

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

Why do you use a version based approach below, and a path based approach here?

@wilzbach
Copy link
Contributor Author

Okay I finally found time to rebase this again and reduce its size. Now there's no version checking anymore and it solely checks whether the DMD/LDC release comes with a DUB binary.

- often interpreted as CWD with obviously problematic effects
- use ${parameter:+word} substitution to only add `:`
  when PATH is not empty (not null)
export LD_LIBRARY_PATH="$ROOT/$1/$libpath:\${LD_LIBRARY_PATH:-}"
export PATH="${DUB_BIN_PATH}${DUB_BIN_PATH:+:}$ROOT/$1/$binpath\${PATH:+:}\${PATH:-}"
export LIBRARY_PATH="$ROOT/$1/$libpath\${LIBRARY_PATH:+:}\${LIBRARY_PATH:-}"
export LD_LIBRARY_PATH="$ROOT/$1/$libpath\${LD_LIBRARY_PATH:+:}\${LD_LIBRARY_PATH:-}"
Copy link
Member

Choose a reason for hiding this comment

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

A trailing : means that ld.so would load shared libraries from the current working directory, thus hiding system wide shared libraries :o.

@MartinNowak
Copy link
Member

Changed a few things, could you have a look @wilzbach.

@MartinNowak MartinNowak assigned wilzbach and unassigned MartinNowak Dec 21, 2017
@wilzbach
Copy link
Contributor Author

Changed a few things, could you have a look @wilzbach.

CI was failing with:

./script/install.sh: line 634: bad substitution: no closing `}' in '}${DUB_BIN_PATH}${DUB_BIN_PATH:+' 

-> I escaped the parameter 😉

A trailing : means that ld.so would load shared libraries from the current working directory, thus hiding system wide shared libraries :o.

Wow good catch. I like the :+: parameter expansion trick. Nice one!

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.

4 participants