-
Notifications
You must be signed in to change notification settings - Fork 169
Conversation
You'll also need these packages (per docs):
|
@akoeplinger cheers, added them in, |
@jchannon sure, cause that doesn't try to run any apps ;) dnu restore needs libcurl e.g. |
gotcha, damn it failed build |
That package is only available starting from Debian Jessie, you should switch to that from wheezy. |
… it all goes wrong its all his fault :)
Failed again |
I think the CI script needs to be modified to strip off the |
Is it taking it from my new folder name I wonder On Thursday, 3 September 2015, Alexander Köplinger [email protected]
|
I've stripped it but i'm not sure thats correct as I think we'll need a tag for coreclr. Happy to undo last commit and then take advice on path forward from MS team |
I think you just need to just strip it at build-app-image.sh#L18 otherwise it'd take the same |
Same in build-all-tags too though |
No, in that case we want it to be |
9f90d22
to
fe97528
Compare
That passed the tests but I think when it validated the beta7-coreclr helloweb/mvc it then used the beta7 dockerfile. I'll leave it up to MS now to pull this branch and make the necessary changes or point our what needs to change and I'll happily do it |
@@ -12,7 +12,7 @@ BASE_IMAGE=$1 | |||
TEST_APP=$2 | |||
TAG=$3 | |||
VERSION=$4 | |||
|
|||
VERSION=$(echo $VERSION | sed -e "s/-coreclr//") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might wanna do this s/-coreclr$//
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or even VERSION=${VERSION%-coreclr}
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually let's extract $SAMPLES_REPO/samples/$VERSION/$TEST_APP
into a variable (it repeats at least twice) and handle it there inline like ".../${VERSION%-coreclr}/..."
@jchannon yeah looks like we need to refactor CI system based on that. In the meanwhile can you please manually verify HelloMvc or HelloWeb works with this image? Can you also please update |
I'll go one even better! Deployed a test app using that Docker image using a tool called http://www.convox.com/ |
@ahmetalpbalkan do you want me to add a coreclr sample here so the tests in this repo pick up the correct HelloWorld samples? Although that might be a catch22 because it wont be able to use the coreclr version until its published but it wont get published as it wont pass the tests here properly 😄 |
@superlogical i see from that you hardcoded the path to dnu/dnx? I've tried building HelloWeb/Mvc locally and it says dnu/dnx isnt in the PATH when it clearly is |
@@ -0,0 +1,25 @@ | |||
FROM debian:jessie |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a later debian release than wheezy with the dependencies we need in it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jchannon can you please check if we still need to compile libuv ourselves in jessie? Historically in #22 we started to compile this ourselves, if the libuv in jessie repos is good enough then we can move all images to jessie in a separate PR.
cc: @akoeplinger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahmetalpbalkan yeah, jessie is still on 0.10 while we need 1.x: https://packages.debian.org/jessie/libuv-dev
|
||
RUN curl -sSL https://raw.githubusercontent.com/aspnet/Home/dev/dnvminstall.sh | DNX_USER_HOME=$DNX_USER_HOME DNX_BRANCH=v$DNX_VERSION sh | ||
RUN bash -c "source $DNX_USER_HOME/dnvm/dnvm.sh \ | ||
&& dnvm upgrade -r coreclr" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better do it like the other versions. This implicitly downloads the latest version of the runtime and doesn't set the alias which is required for the PATH.
RUN bash -c "source $DNX_USER_HOME/dnvm/dnvm.sh \
&& dnvm install $DNX_VERSION -r coreclr -a default \
&& dnvm alias default | xargs -i ln -s $DNX_USER_HOME/runtimes/{}$DNX_USER_HOME/runtimes/default"
I can conform that my application is building and runs correctly after this change.
I have to agree with @ahmetalpbalkan that tagging the image with |
Helloworld samples work with last commit. Good spot @vlesierse |
@jchannon Yeah I hard coded the path to dnu and dnx to get it working. I wish dnu/dnx and dnvm was a single binary |
@superlogical FYI dnu and dnx are already a single binary (there's dnu.cmd that just calls into dnx.exe with some arguments). dnvm needs to be separate and unmanaged code (it's a powershell/bash script), because where would the dnx that you need to download a new dnx come from otherwise? 😄 |
@jchannon this ready to go? |
yup, go go go but i imagine youll have to merge the sample first |
This made me wonder how does this build pass if the sample is not merged. |
because it uses the 1.0.0-beta7 version. thats what i saw in the build log anyway |
|
I think its because of the |
@jchannon for now my suggestion is, just like we trim the -coreclr from the dirname in the CI scripts, we can "sed --in-place" to replace the image name for coreclr samples. Or a better one: For coreclr images, we can clone the repo and copy the samples into a -coreclr folder just like you did here and do the replacement there in the CI scripts. It would look just the same to the script then. What do you think? |
I'm easy, I guess whatever is easiest long term for you to maintain 😄 |
Let's go with the second option if it sounds good to you as well. Thank you so much. @jchannon |
Just to clarify, the cloning already happens as part of the CI build so you want to make a copy of that directory or clone twice one into a samples directory and one into a sample-coreclr directory? I'm wondering if this is a change you guys might have to do? |
@jchannon I think if the tag ends with
so we'll have
to append |
Actually wouldn't it just be easier of we remove the ${VERSION%-coreclr} so On 9 September 2015 at 17:59, Ahmet Alp Balkan [email protected]
|
@jchannon my argument is, for now let's not duplicate samples in the |
BTW if there's anything blocked on this, we can merge and release the image but I prefer waiting on the fix for the CI script. |
@@ -14,6 +14,7 @@ This project is part of ASP.NET 5. You can find samples, documentation, and gett | |||
## Supported tags | |||
|
|||
* [`1.0.0-beta7`, `latest` _(1.0.0-beta7/Dockerfile)_](https://github.com/aspnet/aspnet-docker/blob/master/1.0.0-beta7/Dockerfile) | |||
* [`1.0.0-beta7`, `coreclr` _(1.0.0-beta7/Dockerfile)_](https://github.com/aspnet/aspnet-docker/blob/master/1.0.0-beta7-coreclr/Dockerfile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jchannon this line should just be coreclr-1.0.0-beta7
, I don't think we need to maintain a coreclr
tag for now.
I think its best you finish this off |
OK, merging. Thanks @jchannon |
This PR provides a beta 7 coreclr dockerfile
This addresses #83