Move submodules to NuGet #234

Closed
FunkyLoveCow opened this Issue Apr 4, 2013 · 19 comments

Comments

Projects
None yet
5 participants
@FunkyLoveCow
Contributor

FunkyLoveCow commented Apr 4, 2013

Would it make sense to migrate the SteamKit2 submodule to the NuGet Gallery?

NuGet is available in both Visual Studio and Mono Develop

MonoDevelop: http://community.sharpdevelop.net/blogs/mattward/archive/2013/01/07/MonoDevelopNuGetAddin.aspx
Visual Studio: http://visualstudiogallery.msdn.microsoft.com/27077b70-9dad-4c64-adcf-c7cf6bc9970c

The SteamKit2 library is available here: http://nuget.org/packages/steamkit2

Advantages:

  • Dependencies for SteamKit (protobuf-net) are automatically taken care of
  • Updates to the SteamKit2 library are easily acquired directly from the SteamKit devs
@cwhelchel

This comment has been minimized.

Show comment Hide comment
@cwhelchel

cwhelchel Apr 4, 2013

Contributor

This depends on if there is presently any divergent code added by the SteamBot developers. @everyone Well is there?

Anyway, does NuGet bring down the source or just the assemblies? I can see people who need cough cough crafting that would need the source anyway to add their own customization.

Contributor

cwhelchel commented Apr 4, 2013

This depends on if there is presently any divergent code added by the SteamBot developers. @everyone Well is there?

Anyway, does NuGet bring down the source or just the assemblies? I can see people who need cough cough crafting that would need the source anyway to add their own customization.

@medcat

This comment has been minimized.

Show comment Hide comment
@medcat

medcat Apr 4, 2013

Contributor

there are modifications that we have made to the SteamKit2 source code that is required for SteamBot use. I'm not sure if the modifications we made are in the current version of SteamKit2, but here they are.

Contributor

medcat commented Apr 4, 2013

there are modifications that we have made to the SteamKit2 source code that is required for SteamBot use. I'm not sure if the modifications we made are in the current version of SteamKit2, but here they are.

@FunkyLoveCow

This comment has been minimized.

Show comment Hide comment
@FunkyLoveCow

FunkyLoveCow Apr 4, 2013

Contributor

@cwhelchel It adds the entire SteamKit source/project. It may be useful for crafting, but I'm more interested in the newer enum values for trading mentioned in #227 and possibly some of the user related enums. I haven't made it through all of those yet, but if/when SteamKit can tell the difference between a friend/clan request that'd be nice to get as well.

@redjazz96 From what I see, the difference in that diff is that the two classes are made public. The crypto key looks like it just messed with spacing or white space or similar. The value appears to be the same. Does that sound right?

Contributor

FunkyLoveCow commented Apr 4, 2013

@cwhelchel It adds the entire SteamKit source/project. It may be useful for crafting, but I'm more interested in the newer enum values for trading mentioned in #227 and possibly some of the user related enums. I haven't made it through all of those yet, but if/when SteamKit can tell the difference between a friend/clan request that'd be nice to get as well.

@redjazz96 From what I see, the difference in that diff is that the two classes are made public. The crypto key looks like it just messed with spacing or white space or similar. The value appears to be the same. Does that sound right?

@medcat

This comment has been minimized.

Show comment Hide comment
@medcat

medcat Apr 4, 2013

Contributor

Yes; but we use those classes in the SteamBot.

Contributor

medcat commented Apr 4, 2013

Yes; but we use those classes in the SteamBot.

@cwhelchel

This comment has been minimized.

Show comment Hide comment
@cwhelchel

cwhelchel Apr 6, 2013

Contributor

Also, we could add the JSON library as a NuGet dependency as well.

http://www.nuget.org/packages/Newtonsoft.Json

Contributor

cwhelchel commented Apr 6, 2013

Also, we could add the JSON library as a NuGet dependency as well.

http://www.nuget.org/packages/Newtonsoft.Json

@FunkyLoveCow

This comment has been minimized.

Show comment Hide comment
@FunkyLoveCow

FunkyLoveCow Apr 8, 2013

Contributor

^ That reference is in error. Ignore it.

Contributor

FunkyLoveCow commented Apr 8, 2013

^ That reference is in error. Ignore it.

@medcat

This comment has been minimized.

Show comment Hide comment
@medcat

medcat Apr 30, 2013

Contributor

The other problem is that MonoDevelop on linux is still at 2. something, while that plugin requires 3.something or 4.something, I think.

Contributor

medcat commented Apr 30, 2013

The other problem is that MonoDevelop on linux is still at 2. something, while that plugin requires 3.something or 4.something, I think.

@FunkyLoveCow

This comment has been minimized.

Show comment Hide comment
@FunkyLoveCow

FunkyLoveCow May 22, 2013

Contributor

I'll close this for now based on feedback above.

Contributor

FunkyLoveCow commented May 22, 2013

I'll close this for now based on feedback above.

@FunkyLoveCow

This comment has been minimized.

Show comment Hide comment
@FunkyLoveCow

FunkyLoveCow Jul 16, 2013

Contributor

I'm going to reopen this for further investigation.

Recent SteamKit2 Change

@redjazz96's (Thanks Redjazz) recent commit to SteamKit was accepted giving us access to the RSACrypto and KeyDictionary utility classes that we've been switching to public. Additionally, this has been pushed to NuGet. Perhaps we can either improve how we handle SteamKit updates with this recent change or completely convert it to NuGet.

Contributor

FunkyLoveCow commented Jul 16, 2013

I'm going to reopen this for further investigation.

Recent SteamKit2 Change

@redjazz96's (Thanks Redjazz) recent commit to SteamKit was accepted giving us access to the RSACrypto and KeyDictionary utility classes that we've been switching to public. Additionally, this has been pushed to NuGet. Perhaps we can either improve how we handle SteamKit updates with this recent change or completely convert it to NuGet.

@FunkyLoveCow FunkyLoveCow reopened this Jul 16, 2013

@medcat

This comment has been minimized.

Show comment Hide comment
@medcat

medcat Jul 18, 2013

Contributor

Yes, we can move it completely to nuget.

Contributor

medcat commented Jul 18, 2013

Yes, we can move it completely to nuget.

@FunkyLoveCow

This comment has been minimized.

Show comment Hide comment
@FunkyLoveCow

FunkyLoveCow Jul 18, 2013

Contributor

Awesome. I've started a branch to get it migrated. Hopefully I'll have something by the weekend for this.

Edit: Well, it works in VS but fails on Travis (mono). I've pulled the branch and am going to try a couple other things.

Contributor

FunkyLoveCow commented Jul 18, 2013

Awesome. I've started a branch to get it migrated. Hopefully I'll have something by the weekend for this.

Edit: Well, it works in VS but fails on Travis (mono). I've pulled the branch and am going to try a couple other things.

@FunkyLoveCow

This comment has been minimized.

Show comment Hide comment
@FunkyLoveCow

FunkyLoveCow Jul 19, 2013

Contributor

Anyone else know how to pull git submodules out and replace them with NuGet packages? I've been trying (and succeeding with VS) but with Linux builds it's been failing: https://travis-ci.org/FunkyLoveCow/SteamBot/builds/9252476

Contributor

FunkyLoveCow commented Jul 19, 2013

Anyone else know how to pull git submodules out and replace them with NuGet packages? I've been trying (and succeeding with VS) but with Linux builds it's been failing: https://travis-ci.org/FunkyLoveCow/SteamBot/builds/9252476

@FunkyLoveCow

This comment has been minimized.

Show comment Hide comment
@FunkyLoveCow

FunkyLoveCow Aug 1, 2013

Contributor

Ok, I think I have it working. It builds for me in VS 2010 and according to Travis it's building on Mono.

Can a few other people pull my nuget-migrate branch and make sure it builds appropriately for them before I squash my commits and submit a pull request?

Change log:
In Visual Studio, you will notice that there are only two subprojects now:
Solution Explorer

There are, however, three new directories in the project:
Directories

The .nuget directory is the same as the one you see in the solution explorer. It contains information on which packages to download. The .ci directory contains information on how Travis should build the project. It is not actually used by Visual Studio (or MonoDevelop) or build anything. It is simply Travis control data. The packages directory is not committed (and should not be committed) to GitHub. It is the location where all the nuget packages are downloaded. It contains binary files. It has been explicitly excluded by the .gitignore file.

.travis.yml has been modified to execute the files mentioned in the .ci directory.

.gitignore has been updated to allow the NuGet.exe file and a Microsoft.Build.dll (provided by the Mono team) to facilitate using NuGet. It has also been modified to disallow committing the packages directory.

The existing submodules have all been removed. NuGet will now download SteamKit2, Protobuf-net version 2.0.0.621 and the Newtonsoft.Json packages.

The build script in the .ci directory has been written to compile as before, but I added a parameter to not print out warnings generated by SteamKit2. These were syntactically incorrect cref attribute warnings, and the SteamKit2 team has set their build script to ignore them as well.

Other notes:

  • The .ci stuff is very similar to what the SteamKit team is using. It was very helpful in getting this all to work.
  • The protobuf version mentioned above is not the newest one available in NuGet. Do not force a package update on that package. While the newest one appears to work if you are using Visual Studio, it throws several errors when using Mono. This branch/commit has the correct version associated and it would take user interaction to go to a newer version. It is something to be aware of, but also something you shouldn't encounter if you don't force updates.

TL;DR:
Can a few other people pull my nuget-migrate branch and make sure it builds appropriately for them before I squash my commits and submit a pull request?

Contributor

FunkyLoveCow commented Aug 1, 2013

Ok, I think I have it working. It builds for me in VS 2010 and according to Travis it's building on Mono.

Can a few other people pull my nuget-migrate branch and make sure it builds appropriately for them before I squash my commits and submit a pull request?

Change log:
In Visual Studio, you will notice that there are only two subprojects now:
Solution Explorer

There are, however, three new directories in the project:
Directories

The .nuget directory is the same as the one you see in the solution explorer. It contains information on which packages to download. The .ci directory contains information on how Travis should build the project. It is not actually used by Visual Studio (or MonoDevelop) or build anything. It is simply Travis control data. The packages directory is not committed (and should not be committed) to GitHub. It is the location where all the nuget packages are downloaded. It contains binary files. It has been explicitly excluded by the .gitignore file.

.travis.yml has been modified to execute the files mentioned in the .ci directory.

.gitignore has been updated to allow the NuGet.exe file and a Microsoft.Build.dll (provided by the Mono team) to facilitate using NuGet. It has also been modified to disallow committing the packages directory.

The existing submodules have all been removed. NuGet will now download SteamKit2, Protobuf-net version 2.0.0.621 and the Newtonsoft.Json packages.

The build script in the .ci directory has been written to compile as before, but I added a parameter to not print out warnings generated by SteamKit2. These were syntactically incorrect cref attribute warnings, and the SteamKit2 team has set their build script to ignore them as well.

Other notes:

  • The .ci stuff is very similar to what the SteamKit team is using. It was very helpful in getting this all to work.
  • The protobuf version mentioned above is not the newest one available in NuGet. Do not force a package update on that package. While the newest one appears to work if you are using Visual Studio, it throws several errors when using Mono. This branch/commit has the correct version associated and it would take user interaction to go to a newer version. It is something to be aware of, but also something you shouldn't encounter if you don't force updates.

TL;DR:
Can a few other people pull my nuget-migrate branch and make sure it builds appropriately for them before I squash my commits and submit a pull request?

@Lagg

This comment has been minimized.

Show comment Hide comment
@Lagg

Lagg Aug 1, 2013

Contributor

I'm really not comfortable with untrusted binaries being in a distribution. Someone like @Jessecar96 or @geel9 need to grab the dll and nuget.exe from somewhere and preferably sign it with PGP. It's too easy for binaries to be stealthily modified in a pull request and merged without people noticing. I've seen it happen before. Please don't take this as me not trusting you @FunkyLoveCow, I do. It's just a manner of precaution that is standard procedure when putting third party binaries in version control.

Contributor

Lagg commented Aug 1, 2013

I'm really not comfortable with untrusted binaries being in a distribution. Someone like @Jessecar96 or @geel9 need to grab the dll and nuget.exe from somewhere and preferably sign it with PGP. It's too easy for binaries to be stealthily modified in a pull request and merged without people noticing. I've seen it happen before. Please don't take this as me not trusting you @FunkyLoveCow, I do. It's just a manner of precaution that is standard procedure when putting third party binaries in version control.

@medcat

This comment has been minimized.

Show comment Hide comment
@medcat

medcat Aug 1, 2013

Contributor

@Lagg like @Jessecar96 and @geel9 are a) gonna be responsible with a PGP key, and b) gonna get their key signed by anyone.

@FunkyLoveCow, with your .ci/build.sh file, can't you just simplify the whole thing down to

+xbuild /p:NoWarn=1584 /property:Configuration=Debug /property:Platform="Any CPU" SteamBot.sln 
exit $?
Contributor

medcat commented Aug 1, 2013

@Lagg like @Jessecar96 and @geel9 are a) gonna be responsible with a PGP key, and b) gonna get their key signed by anyone.

@FunkyLoveCow, with your .ci/build.sh file, can't you just simplify the whole thing down to

+xbuild /p:NoWarn=1584 /property:Configuration=Debug /property:Platform="Any CPU" SteamBot.sln 
exit $?
@FunkyLoveCow

This comment has been minimized.

Show comment Hide comment
@FunkyLoveCow

FunkyLoveCow Aug 1, 2013

Contributor

@redjazz96 It looks like it yes. I took that script from the SteamKit guys. They use the function to also run some automated tests and retry the CI build on certain failures. We can leave it how it is in case we want to add extra steps to the build (see option 2 below)

@Lagg, No offense taken. I can understand the caution. I'll take a look and see if I can find an acceptable alternative. Right now I've got a couple ideas, though I don't know if any of them are viable.

  • Automatically redownload NuGet.exe on build. I'd assume that Microsoft has this option built into VS, but I don't know if it'd be Mono compatible.
  • We could build a check into the .ci script to look for NuGet.exe or Microsoft.Build.dll and if they are seen fail the build. The pull request could still be made, but the failure message should cause one of the repo managers to question why they are merging these particular items.
  • The entire .nuget directory could be a subrepository. The only thing that should change in there at this point is the .targets file and then only if we add additional steps to the build process. Version management is kept in the solution .packages files and wouldn't be affected by this subrepository change.

My thoughts on each:

  • The first one would be the best. If we can automatically pull it using the IDE, I think this would be the safest. Do you?
  • Adding in the technical check may be a good idea anyway. However, there are legitimate ways to prevent a CI build from occurring. As a policy, we should probably not accept pull requests that didn't pass a CI build.
  • I just pulled out all of the submodules. I'd rather not add one back in. However, this particular submodule would have very little change to it (compared to SteamKit's semi-regular updates). Any pull requests to it would be able to be checked easily. With only 4 files in the entire directory it should be fairly obvious that one of the binary files changed.

Alternatively, we could rely on the peer reviews that we've been having when pull requests get submitted.

Contributor

FunkyLoveCow commented Aug 1, 2013

@redjazz96 It looks like it yes. I took that script from the SteamKit guys. They use the function to also run some automated tests and retry the CI build on certain failures. We can leave it how it is in case we want to add extra steps to the build (see option 2 below)

@Lagg, No offense taken. I can understand the caution. I'll take a look and see if I can find an acceptable alternative. Right now I've got a couple ideas, though I don't know if any of them are viable.

  • Automatically redownload NuGet.exe on build. I'd assume that Microsoft has this option built into VS, but I don't know if it'd be Mono compatible.
  • We could build a check into the .ci script to look for NuGet.exe or Microsoft.Build.dll and if they are seen fail the build. The pull request could still be made, but the failure message should cause one of the repo managers to question why they are merging these particular items.
  • The entire .nuget directory could be a subrepository. The only thing that should change in there at this point is the .targets file and then only if we add additional steps to the build process. Version management is kept in the solution .packages files and wouldn't be affected by this subrepository change.

My thoughts on each:

  • The first one would be the best. If we can automatically pull it using the IDE, I think this would be the safest. Do you?
  • Adding in the technical check may be a good idea anyway. However, there are legitimate ways to prevent a CI build from occurring. As a policy, we should probably not accept pull requests that didn't pass a CI build.
  • I just pulled out all of the submodules. I'd rather not add one back in. However, this particular submodule would have very little change to it (compared to SteamKit's semi-regular updates). Any pull requests to it would be able to be checked easily. With only 4 files in the entire directory it should be fairly obvious that one of the binary files changed.

Alternatively, we could rely on the peer reviews that we've been having when pull requests get submitted.

@Lagg

This comment has been minimized.

Show comment Hide comment
@Lagg

Lagg Aug 1, 2013

Contributor

I like option 1. Mono incompatibility isn't much issue at the moment since this is aimed at people using VS anyway as far as I can tell. Those using mono can probably find the required libs themselves. I commonly do this when building C# code on non-Windows systems. It's not too bad of a task. There aren't too many dependencies for this project either, so that helps.

Contributor

Lagg commented Aug 1, 2013

I like option 1. Mono incompatibility isn't much issue at the moment since this is aimed at people using VS anyway as far as I can tell. Those using mono can probably find the required libs themselves. I commonly do this when building C# code on non-Windows systems. It's not too bad of a task. There aren't too many dependencies for this project either, so that helps.

@FunkyLoveCow

This comment has been minimized.

Show comment Hide comment
@FunkyLoveCow

FunkyLoveCow Aug 2, 2013

Contributor

@Lagg Option one is actually very easy if using Visual Studio. It's changing a false to a true and then restarting Visual Studio if you changed it while the IDE was open.

Mono is not so easy. They have to manually download Nuget from Codeplex and the DLL included right now. The problems I'm seeing with this are that Codeplex doesn't seem to have an easy way to get the most recent version of Nuget automatically with a curl/wget script and the dll comes from a Mono for Windows install.

Relaying this information to a person is easy. We provide directions, screenshots, and tell them to RTFM. They do and get developing.

The problem is doing this so that the CI stuff works. I can't find an easy way to get either file. Without those files Travis will fail 100% of the time because it can't include Steamkit, protobuf or Newton.json.

Edit: Completely failed to tell you what needs to change.
In the .nuget/NuGet.targets file, change the <DownloadNuGetExe Condition=" '$(DownloadNuGetExe)' == '' ">false</DownloadNuGetExe> to <DownloadNuGetExe Condition=" '$(DownloadNuGetExe)' == '' ">true</DownloadNuGetExe> (roughly around line 16)

Contributor

FunkyLoveCow commented Aug 2, 2013

@Lagg Option one is actually very easy if using Visual Studio. It's changing a false to a true and then restarting Visual Studio if you changed it while the IDE was open.

Mono is not so easy. They have to manually download Nuget from Codeplex and the DLL included right now. The problems I'm seeing with this are that Codeplex doesn't seem to have an easy way to get the most recent version of Nuget automatically with a curl/wget script and the dll comes from a Mono for Windows install.

Relaying this information to a person is easy. We provide directions, screenshots, and tell them to RTFM. They do and get developing.

The problem is doing this so that the CI stuff works. I can't find an easy way to get either file. Without those files Travis will fail 100% of the time because it can't include Steamkit, protobuf or Newton.json.

Edit: Completely failed to tell you what needs to change.
In the .nuget/NuGet.targets file, change the <DownloadNuGetExe Condition=" '$(DownloadNuGetExe)' == '' ">false</DownloadNuGetExe> to <DownloadNuGetExe Condition=" '$(DownloadNuGetExe)' == '' ">true</DownloadNuGetExe> (roughly around line 16)

@rpappa

This comment has been minimized.

Show comment Hide comment
@rpappa

rpappa Nov 25, 2013

Can't this be closed now?

rpappa commented Nov 25, 2013

Can't this be closed now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment