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

use dotnet core #10

Open
wants to merge 6 commits into
base: master
from

Conversation

@friism
Copy link
Collaborator

commented Dec 4, 2016

No description provided.

@tkrotoff

This comment has been minimized.

Copy link

commented Dec 19, 2016

(Could not open an issue on your repo)
I've tried you fork and got this with heroku logs:

heroku[web.1]: Starting process with command `ASPNETCORE_URLS='http://+:31719' dotnet ./app.dll -c Release`
app[web.1]: 
app[web.1]: Unhandled Exception: System.FormatException: The short switch '-c' is not defined in the switch mappings.

Sure the -c switch exists? see https://docs.microsoft.com/en-us/dotnet/articles/core/tools/dotnet

Edit:
I'm using this Procfile instead and it works fine:
web: dotnet ./app.dll --urls http://+:$PORT

The Release flag should be on the dotnet publish, something like: dotnet publish --output out --configuration Release, see https://docs.microsoft.com/en-us/dotnet/articles/core/tools/dotnet-publish

@friism

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 19, 2016

Interesting, it doesn't complain when I test in a heroku run:

Microsoft.DotNet.PlatformAbstractions.dll              System.ComponentModel.Primitives.dll
~ $ dotnet app.dll -c Release
Hosting environment: Production
Content root path: /app
Now listening on: http://localhost:5000
Application started. Press Ctrl+C to shut down.
^CApplication is shutting down...
~ $ dotnet app.dll -c Debug
Hosting environment: Production
Content root path: /app
Now listening on: http://localhost:5000
Application started. Press Ctrl+C to shut down.
^CApplication is shutting down...

I think you're right though, it doesn't make sense here, I'll remove it.

@tkrotoff

This comment has been minimized.

Copy link

commented Dec 19, 2016

btw, you can remove $DNU_FLAGS from here, I believe it is a relic from the past :)

@friism

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 19, 2016

@tkrotoff done, thx

@tkrotoff

This comment has been minimized.

Copy link

commented Dec 21, 2016

I prefer to document the error I've encountered here.

I had a hard time getting rid of the following error One or more errors occurred. (The process cannot access the file because it is being used by another process.) with friism/dotnet-buildpack. Of course this error did not happen locally or with a Docker image.

// ...
remote:    [0] multi react 76 bytes {12} [built]
remote:    [0] multi polyfills 40 bytes {11} [built]
remote:    [0] multi moment 40 bytes {9} [built]
remote:    [0] multi mobx 40 bytes {8} [built]
remote:     + 792 hidden modules
remote: Child html-webpack-plugin for "index.html":
remote:         + 3 hidden modules
remote: Child extract-text-webpack-plugin:
remote:         + 8 hidden modules
 66% 617/658 buiOne or more errors occurred. (The process cannot access the file because it is being used by another process.)
remote:  !     Push rejected, failed to compile .NET Core app.
remote:
remote:  !     Push failed
remote: Verifying deploy....
remote:
remote: !	Push rejected to [...].

I use webpack for the client side and my project.json looks like this:

"scripts": {
  "prepublish": [ "npm install", "npm run clean", "npm run build:client:prod" ],
  "postpublish": [ "dotnet publish-iis --publish-folder %publish:OutputPath% --framework %publish:FullTargetFramework%" ]
}

package.json:

"scripts": {
  "clean": "npm-run-all --parallel clean:*",
  "clean:server": "rimraf ./bin ./obj",
  "clean:client": "rimraf ./wwwroot/*",
  "build:client:prod": "cross-env NODE_ENV=production webpack -p --progress"
}

The problem was the --progress option passed to webpack. Without I don't get the error. Don't ask me why or how, after 2 days spent on this I have no clue.

@tkrotoff

This comment has been minimized.

Copy link

commented Dec 21, 2016

@friism

Your buildpack installs Grunt, Bower and Gulp globally.

This is not a good practice. These dependencies should be installed locally using package.json and managed with scripts or by calling node_modules/.bin/myclitool.
(You are also doing this "mistake" in your docker example).

Otherwise what about the buildpack installing globally Webpack, Browserify, Rollup, Compass, SASS, ..., the next cool new command line tool? A buildpack should be agnostic and heroku-buildpack-nodejs for example is.

Btw Bower is kind of obsolete: 1, 2, 3.

Anyway, your buildpack works wonderfully, better than the others, thanks a lot for your work.

@friism

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 21, 2016

@tkrotoff I'm not particularly happy about adding the javascript stuff either, but the problem is that almost all of the default .NET samples assume that they exist on the dev/build system. My docker sample is a good example: I didn't build it, I just copied sample code from the EntityFramework docs and tweaked them a little: https://github.com/aspnet/EntityFramework.Docs/tree/master/samples/core/GetStarted/AspNetCore/AspNetCore.NewDb

I could remove it from the buildpack, but then the buildpack would be able to build only a small subset of projects.

Also note that Bower and Grunt are only available when the buildpack is building, they're not added to the runtime slug. If you do heroku run bash and look around in the running app, there's no Javascript stuff slowing down app deployment, etc.

@tkrotoff

This comment has been minimized.

Copy link

commented Dec 23, 2016

@friism
The Procfile provided by default is: web: ASPNETCORE_URLS='http://+:$PORT' dotnet ./app.dll

It should be web: ASPNETCORE_URLS='http://+:$PORT' dotnet ./app.dll --urls http://+:$PORT, see friism#1

tkrotoff added a commit to tkrotoff/dotnet-buildpack that referenced this pull request Dec 23, 2016

tkrotoff and others added some commits Dec 23, 2016

@friism

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 23, 2016

@tkrotoff thanks, I've opened up for issues on the repo on my account.

@mplacona

This comment has been minimized.

Copy link

commented Jan 24, 2017

Tested this and it works fine. This should get merged!

@mpdeimos

This comment has been minimized.

Copy link

commented Jan 24, 2017

I can just second that. Works like a charm, I just had to specify a Procfile together with a .deployment file to target my app dll instead of the test dll: https://github.com/mpdeimos/strava-weather/blob/master/Procfile

@kharlamov

This comment has been minimized.

Copy link

commented Feb 27, 2017

Any way we can get some movement on all of these changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.