Skip to content

Conversation

vladbarosan
Copy link

@vladbarosan vladbarosan commented Oct 17, 2017

Trying to address #105

Copy link
Contributor

@vishrutshah vishrutshah left a comment

Choose a reason for hiding this comment

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

Seems like this is not the right fix based on the PR Azure/azure-rest-api-specs#1884 that is having the other issue.

Let's talk to Johannes and find out what .NET core version are we supposed to use and then may be need to change the command as well.

@vladbarosan vladbarosan changed the title Lock to version 1.1.0 of AutoRest Update to autorest 2 Oct 18, 2017
@vladbarosan vladbarosan force-pushed the fix-build branch 9 times, most recently from 652f4c3 to be8cb3e Compare October 19, 2017 19:59
.travis.yml Outdated
- npm install -g npm@'>=5.4.0'
- curl https://packages.microsoft.com/keys/microsoft.asc | gpg --dearmor > microsoft.gpg
- sudo mv microsoft.gpg /etc/apt/trusted.gpg.d/microsoft.gpg
- sudo sh -c 'echo "deb [arch=amd64] https://packages.microsoft.com/repos/microsoft-ubuntu-trusty-prod trusty main" > /etc/apt/sources.list.d/dotnetdev.list'
Copy link
Contributor

Choose a reason for hiding this comment

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

try out sudo apt-get install nodejs specific version you want

Copy link
Author

Choose a reason for hiding this comment

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

That wont work properly. To properly install a specific major version of nodejs you need to run a remote script like https://deb.nodesource.com/setup_8.x and after that to run sudo apt-get install nodejs.
We can do both, only that that way we won't have much granularity in the version used ( in case we care )

<OutputType>Exe</OutputType>
<TargetFrameworks>netcoreapp1.0</TargetFrameworks>
<TargetFramework>netcoreapp1.0</TargetFramework>
<TargetFramework>netcoreapp2.0</TargetFramework>
Copy link
Contributor

Choose a reason for hiding this comment

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

try to move this also to netstandard2.0 if we can

Copy link
Author

Choose a reason for hiding this comment

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

For exe type we cannot use netstandard2.0 ( intended only for libraries). Since we are using this dill like an executable we need to stay to netcoreapp2.0

<Import Project="$(common)project-xunittest.proj" />

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>netcoreapp1.0</TargetFramework>
<TargetFramework>netcoreapp2.0</TargetFramework>
Copy link
Contributor

Choose a reason for hiding this comment

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

netstandard2.0

Copy link
Author

Choose a reason for hiding this comment

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

Same as previous comment. since the unit test project is an exe we need to keep netcoreapp2.0

@vishrutshah vishrutshah dismissed their stale review October 19, 2017 20:42

I'll let @veronicagg look at it in details

Copy link
Contributor

@veronicagg veronicagg left a comment

Choose a reason for hiding this comment

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

If you're planning on releasing, please update the ChangeLog in this or a follow-up PR.
Please follow Release steps from our internal docs (btw, we should probably post them in github)

.travis.yml Outdated
sudo apt-get install dotnet-dev-1.0.0-rc4-004769 -y
- rm -rf ~/.nvm && git clone https://github.com/creationix/nvm.git ~/.nvm && (cd ~/.nvm && git checkout `git describe --abbrev=0 --tags`) && source ~/.nvm/nvm.sh && nvm install 7.10.0
- npm install -g npm@'>=5.4.0'
- curl https://packages.microsoft.com/keys/microsoft.asc | gpg --dearmor > microsoft.gpg
Copy link
Contributor

Choose a reason for hiding this comment

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

you may want to add a comment on why these lines are needed and maybe a pointer to where you got the pre-reqs from

@vladbarosan vladbarosan force-pushed the fix-build branch 2 times, most recently from 5a622d3 to 62a4db9 Compare October 20, 2017 18:22
@vladbarosan vladbarosan changed the title Update to autorest 2 Update to autorest 2 and dotnet 2.0 Oct 20, 2017
@vladbarosan vladbarosan merged commit 48ea209 into Azure:master Oct 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants