Add 'ldc-nightly' install target by JohanEngelen · Pull Request #282 · dlang/installer

JohanEngelen · 2017-12-27T15:16:36Z

This PR installs ldc-nightly in e.g. "ldc-8c0abd52-20171222"
(DMD installs nightlies in e.g. "dmd-master-2017-12-27")

LDC does not have a LATEST_NIGHTLY file online, so I opted for just downloading the nightlies release page and grepping for what the binary package name should look like.

Tested on OSX.

dlang-bot · 2017-12-27T15:16:37Z

Thanks for your pull request and interest in making D better, @JohanEngelen! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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

LGTM. I will add it to run.dlang.io once this is merged and deployed to dlang.org

wilzbach · 2017-12-27T22:49:58Z

+        ldc-nightly)
+            local CIurl=https://github.com/ldc-developers/ldc/releases/tag/CI
+            logV "Finding latest ldc-nightly binary package (at $CIurl)."
+            local package="$(curl -s $CIurl | grep -o "ldc2-[0-9a-f]*-$OS-$ARCH-[0-9]*.tar.xz" | head -n 1)"


Shellcheck error:

In script/install.sh line 427: local package="$(curl -s $CIurl | grep -o "ldc2-[0-9a-f]*-$OS-$ARCH-[0-9]*.tar.xz" | head -n 1)" ^-- SC2155: Declare and assign separately to avoid masking return values.

See: https://github.com/koalaman/shellcheck/wiki/SC2155

How do you want me to fix this?
local -r silences the warning; i don't particularly care about error codes, because we handle the error elsewhere. Still, it might be nice for the user to see the raw error.. ?

I think both local -r and using a separate line for the definition work though the latter seems to be the more prevalent style in this script, so I would opt for that one.

wilzbach · 2017-12-27T23:00:02Z

Tested on OSX.

And now Linux ;-)
Too increase coverage, you might want to add ldc-nightly to the travis.sh test script.

wilzbach · 2017-12-28T10:49:52Z

Thanks. As @MartinNowak is the author and "owner" of this install script, I would wait for his feedback/approval before merging. I can't deploy it anyways :/

PetarKirov

Thanks for the nice work @JohanEngelen, LGTM!

PetarKirov · 2017-12-28T11:09:28Z

+            local CIurl=https://github.com/ldc-developers/ldc/releases/tag/CI
+            logV "Finding latest ldc-nightly binary package (at $CIurl)."
+            local package
+            package="$(curl -s $CIurl | grep -o "ldc2-[0-9a-f]*-$OS-$ARCH-[0-9]*.tar.xz" | head -n 1)"


Hopefully we can replace this with a more robust way of getting the latest CI build, but we can live with it for now.

The uploads are not sorted and old ones aren't deleted, so that results in a random artifact, not the latest one.
Who's updating the sha of the CI tag btw?

(nice, you spotted the problem too)

PetarKirov · 2017-12-28T11:10:42Z

Nit: can you squash the "Fix [..] warning" commit into the first one? (Once it passes the CI tests.)

MartinNowak · 2017-12-29T14:00:09Z

+            local CIurl=https://github.com/ldc-developers/ldc/releases/tag/CI
+            logV "Finding latest ldc-nightly binary package (at $CIurl)."
+            local package
+            package="$(curl -s $CIurl | grep -o "ldc2-[0-9a-f]*-$OS-$ARCH-[0-9]*.tar.xz" | head -n 1)"


Please use fetch instead of curl so there is some retry logic.

MartinNowak · 2017-12-29T14:01:15Z

            COMPILER="ldc-$(fetch $url)"
            ;;
+        ldc-nightly)
+            local CIurl=https://github.com/ldc-developers/ldc/releases/tag/CI


Weird variable name, how about url or ci_url instead?

MartinNowak · 2017-12-29T14:04:11Z

            logV "Determining latest ldc-beta version ($url)."
            COMPILER="ldc-$(fetch $url)"
            ;;
+        ldc-nightly)


I'd say we should at least also resolve ldc-2017222 so it's possible to install a compiler from a specific date, quite useful for bisecting an issue.
I'm not sure whether you actually plan to archive your nightly builds.

I also don't know whether we'll archive the nightlies.
This is a new feature though, so please just merge this.

(they aren't "nightlies" btw, it's a new build after each git commit)

Perhaps we should name it ldc-latest-ci-build?

I don't like the term 'nightly' either. I guess for DMD they are indeed proper nightlies with no difference compared to regular builds. For us, they are the latest successful CI builds, which means a revision may not be available for all platforms (sporadic failures, timeouts...), not available at the same time (Windows x64/multilib and OSX packages taking quite a bit longer than the Linux x64 one), and particularly, they feature enabled LLVM+LDC assertions (well, currently not for Windows, due to AppVeyor slowness), increasing compile times over a normal release package.

JohanEngelen · 2017-12-29T14:10:00Z

Wait with merging, this does the wrong thing.

Edit: I thought we only keep the latest nightly on that "CI" release page, but apparently new builds are appended. So we cannot just do a grep for the package name, because that would just get the package with the alphabetically first githash. So this PR needs a big rework.

PetarKirov · 2017-12-29T14:32:15Z

@JohanEngelen as mentioned in ldc-developers/ldc#1883 (comment), I think the best solution would be to make your CircleCI job push commits to https://github.com/ldc-developers/ldc-developers.github.io, updating a file like LATEST_CI.

MartinNowak · 2017-12-30T23:24:09Z

Looking at this you might want to add ldc-lts before anything else ;).
Shouldn't the other one always be the latest commit on master?
How about ldc-edge resolving to ldc-$(git describe) (e.g. ldc-1.7.0-beta1-18-gf86e33f1).
Indeed it would be cleaner if you could push LATEST_EDGE/CI to that page or to the CI release page.

MartinNowak · 2017-12-30T23:35:52Z

At best you keep the uploads for a week, so found issues can be reproduced.
Maybe directly using CircleCI's artifacts would be cleaner? Though they don't seem to provide any delete option.

wilzbach · 2018-02-15T02:04:26Z

@JohanEngelen did you manage to write such a LATEST_CI file?

JohanEngelen · 2018-02-15T23:32:11Z

@wilzbach Haven't looked further into this. It's on the backburner...

CyberShadow · 2018-11-25T14:59:26Z

+            local ci_url=https://github.com/ldc-developers/ldc/releases/tag/CI
+            logV "Finding latest ldc-nightly binary package (at $ci_url)."
+            local package
+            package="$(fetch $ci_url | grep -o "ldc2-[0-9a-f]*-$OS-$ARCH-[0-9]*.tar.xz" | head -n 1)"


Surely querying the GitHub API is not going to be any worse than scraping HTML...

CyberShadow · 2018-11-25T15:03:19Z

Wait with merging, this does the wrong thing.

Check if it's possible to formulate a GitHub API query for this. Even if parsing the result with just bash is infeasible, having a dependency on jq seems reasonable.

wilzbach · 2018-11-25T18:35:06Z

The GitHub API is heavily rate-limited. Using it will lead to failures on heavily used CIs like Travis.

CyberShadow · 2018-11-25T18:38:43Z

Oh, because it will share limits with the IPs of CI services?

That makes sense, but scraping HTML is still fragile. CI services generally allow adding secrets to the account, so an API authentication token can be used there. (Or, just move the logic elsewhere...)

JohanEngelen · 2018-11-25T18:54:44Z

I have completely stopped working on this btw. Feel free to take over.

CyberShadow · 2018-11-25T19:00:33Z

I really don't know that much about LDC development.

I could write a tiny web service that returns the needed information from the GitHub API, if it helps. But, I can only guarantee that its availability will be as good as e.g. forum.dlang.org, which is probably around only 99%.

wilzbach · 2018-11-25T19:19:59Z

This would help a lot for this issue.

CyberShadow · 2018-11-25T19:20:33Z

Cool. Will you do the other part if I do that?

See dlang/installer#282 (comment)

JohanEngelen · 2018-12-04T22:51:51Z

@wilzbach @CyberShadow The date has been removed from the filename. See: https://github.com/ldc-developers/ldc/releases/tag/CI

Edit: wait... how does Vlads script determine which is the newest....??? I sense I did something dumb here...

kinke · 2018-12-05T00:23:29Z

Indeed it would be cleaner if you could push LATEST_EDGE/CI to that page or to the CI release page

The main difficulty is that we are talking about 6 artifacts, published by 5 independent jobs, from 3 different CI services. So it's a timing issue and a question of reliability, as some jobs time out not too seldomly or randomly fail, unfortunately.

I guess there's no real way around querying the GitHub API and checking the timestamps of the files. Having a proxy do that (and cache it for some minutes or so) to bypass the rate limit would be nice, of course.

Edit: Well we could theoretically have a LATEST_CI_<platform> file for all platforms.

CyberShadow · 2018-12-05T03:57:51Z

The date has been removed from the filename.

Updated script to show only the hash.

Edit: wait... how does Vlads script determine which is the newest....???

The release ID apparently doesn't change when new files are uploaded, so the data is always at /repos/ldc-developers/ldc/releases/8840125.

Having a proxy do that (and cache it for some minutes or so) to bypass the rate limit would be nice, of course.

GitHub does not count 301 replies against the rate limit, which means the script will always provide the latest data with no rate limit.

kinke · 2018-12-05T10:37:03Z

Can you show the source of the script? I only see the result and wonder how it works. It's almost certain that the Linux package is available earlier than the OSX one, so if it just returns the hash of the latest Linux package, the according OSX package may not be available yet. It needs to be per-platform (something like ?platform=linux-x86_64).

JohanEngelen · 2018-12-09T15:46:32Z

Well we could theoretically have a LATEST_CI_ file for all platforms.

@kinke That doesn't sound all that bad and would simplify things a lot I feel.

kinke · 2018-12-09T17:07:40Z

Not for us, as 3 CI services need to be able to push to that repo, requiring a token to do so, which could even be abused by some malevolent PR etc.
I guess @CyberShadow's implementation just needs an additional handful of lines, taking a platform input param and filtering the artifact filenames before sorting by upload date and extracting the git hash.

CyberShadow · 2018-12-09T21:34:40Z

I changed it to just dump all URLs, so that you can grep it as needed. Does that help?

JohanEngelen · 2018-12-10T07:33:50Z

@CyberShadow Look at the CI release page now. The problem is getting the URL belonging to the latest packages. (getting the urls is not the problem).

CyberShadow · 2018-12-10T07:41:15Z

The GitHub server is replying with a "304 Not Modified" to a request with an "If-Modified-Since" of "Tue, 04 Dec 2018 22:50:54 GMT", for a resource that very obviously has been modified within the past week. Looks like a bug in their API.

Edit: sent a support request.

CyberShadow · 2018-12-10T22:58:35Z

Caching issue was fixed (switched to ETag-based caching at GitHub support's suggestion). What next, should I filter the assets to bin them by the hash and only show the last one per hash?

Can you show the source of the script?

Yes, sorry, here it is:
http://thecybershadow.net/d/github-ldc/index.txt

Library used is https://github.com/milo/github-api + milo/github-api#24.

JohanEngelen · 2018-12-10T23:32:52Z

only show the last one per hash?

I'd say only show the last one per arch.

CyberShadow · 2018-12-10T23:55:29Z

Oops, right. How's this?

JohanEngelen · 2018-12-11T20:51:30Z

Looks good to me :)

kinke · 2018-12-11T21:05:22Z

Yeah, thx a lot, seems to work fine.

Tested on mac and linux. (x86_64)

JohanEngelen · 2018-12-11T23:11:53Z

OK, I've fixed the script. It now works on my mac and on a Ubuntu server too!

JohanEngelen · 2018-12-11T23:12:48Z

rename ldc-nightly to ldc-latest-ci? Or keep the name for uniformity with DMD's?
As @kinke noted: it is a different build than a release build. So perhaps it's not so bad if it has a different name than -nightly...

JohanEngelen · 2018-12-13T16:48:12Z

@wilzbach @kinke @CyberShadow I'm happy with it, it works well for me. Good to merge?

kinke · 2018-12-13T17:02:01Z

Yep, lgtm now.

PetarKirov

Looks good to me, nice work everyone!

wilzbach · 2018-12-20T09:34:18Z

The script needs manual deployment as it needs to be signed, but I just did so.
It should be available on Travis etc. now, e.g.

d: ldc-latest-ci

wilzbach · 2018-12-20T09:35:25Z

Also, I forgot to say thanks to everyone involved in this effort!

@JohanEngelen @kinke you might want to open a thread in the NG, s.t. people know about this and actually enable it in their CIs.

JohanEngelen mentioned this pull request Dec 27, 2017

Integrate CI builds with dlang/install.sh ldc-developers/ldc#1883

Closed

wilzbach approved these changes Dec 27, 2017

View reviewed changes

PetarKirov approved these changes Dec 28, 2017

View reviewed changes

JohanEngelen force-pushed the ldcnightly branch from 5de9ef5 to 63ffff6 Compare December 28, 2017 12:13

MartinNowak reviewed Dec 29, 2017

View reviewed changes

JohanEngelen force-pushed the ldcnightly branch from 63ffff6 to e9061bc Compare December 29, 2017 14:06

JohanEngelen changed the title ~~Add 'ldc-nightly' install target~~ [WIP] Add 'ldc-nightly' install target Dec 29, 2017

dlang-bot added the WIP label Dec 29, 2017

dlang-bot added needs rebase and removed needs rebase labels Jan 1, 2018

CyberShadow reviewed Nov 25, 2018

View reviewed changes

JohanEngelen added a commit to ldc-developers/ldc that referenced this pull request Dec 3, 2018

Remove date from nightly upload package to github CI release

70e731a

See dlang/installer#282 (comment)

kinke pushed a commit to ldc-developers/ldc that referenced this pull request Dec 4, 2018

Remove date suffix from GitHub CI release artifact filenames (#2930)

4ef1560

See dlang/installer#282 (comment)

Fix script to work with date-less package names

49107d3

Tested on mac and linux. (x86_64)

JohanEngelen added 2 commits December 12, 2018 00:36

Rename ldc-nightly to ldc-latest-ci

54b651e

Fix travis test

d8dba7d

PetarKirov approved these changes Dec 13, 2018

View reviewed changes

wilzbach removed the WIP label Dec 20, 2018

wilzbach merged commit 6f477a5 into dlang:master Dec 20, 2018

JohanEngelen deleted the ldcnightly branch December 20, 2018 16:41

wilzbach mentioned this pull request Jan 2, 2019

Add ldc-latest-ci to the install script information dlang/dlang.org#2538

Merged

Uh oh!

Conversation

JohanEngelen commented Dec 27, 2017

Uh oh!

dlang-bot commented Dec 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Dec 27, 2017

Uh oh!

wilzbach commented Dec 28, 2017

Uh oh!

PetarKirov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetarKirov commented Dec 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinke Nov 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JohanEngelen commented Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PetarKirov commented Dec 29, 2017

Uh oh!

MartinNowak commented Dec 30, 2017

Uh oh!

MartinNowak commented Dec 30, 2017

Uh oh!

wilzbach commented Feb 15, 2018

Uh oh!

JohanEngelen commented Feb 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyberShadow commented Nov 25, 2018

Uh oh!

wilzbach commented Nov 25, 2018

Uh oh!

CyberShadow commented Nov 25, 2018

Uh oh!

JohanEngelen commented Nov 25, 2018

Uh oh!

CyberShadow commented Nov 25, 2018

Uh oh!

wilzbach commented Nov 25, 2018

Uh oh!

dlang-bot commented Dec 27, 2017 •

edited

Loading

PetarKirov commented Dec 28, 2017 •

edited

Loading

kinke Nov 28, 2018 •

edited

Loading

JohanEngelen commented Dec 29, 2017 •

edited

Loading

JohanEngelen commented Dec 4, 2018 •

edited

Loading

kinke commented Dec 5, 2018 •

edited

Loading

kinke commented Dec 9, 2018 •

edited

Loading

CyberShadow commented Dec 10, 2018 •

edited

Loading

JohanEngelen commented Dec 11, 2018 •

edited

Loading