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

Enable building on linux without project file changes #1930

Merged
merged 8 commits into from
Jan 31, 2019
Merged

Enable building on linux without project file changes #1930

merged 8 commits into from
Jan 31, 2019

Conversation

Vogel612
Copy link
Contributor

This enables building via cake on linux without requiring adjustments on
other platforms, fixes #1745

This enables building via cake on linux without requiring adjustments on
other platforms, fixes #1745
@ryangribble
Copy link
Contributor

ryangribble commented Jan 21, 2019

Hey @Vogel612, this is cool for building locally on a mac/Linux but the only thing is that on travisCI Mac and Linux builds we actually DO build the full framework as the build agent is setup with mono

So maybe rather than hardcoding noframework=true in build.sh this should be passed in so we can set false in travis?

Also I think the tool install step of the build might still be a problem on Mac/Linux if you don't have mono as it uses nuget (from mono) to pull down gitversion doesn't it ?

@Vogel612
Copy link
Contributor Author

Vogel612 commented Jan 21, 2019

Also I think the tool install step of the build might still be a problem on Mac/Linux if you don't have mono as it uses nuget (from mono) to pull down gitversion doesn't it ?

Yes, the tool install step requires nuget. GitVersion is then executed through mono (cf. tools/gitversion_wrapper.sh).

So maybe rather than hardcoding noframework=true in build.sh this should be passed in so we can set false in travis?

I'm happy to remove the hardcoding there.


Currently I'm building locally by sidestepping the GitVersion invocation and returning a hardcoded JSON because neither mono nor dotnet invoke the downloaded binary without error. (mono segfaults and dotnet is missing a library).
I did install nuget, which depends on mono to fix the step in the first place.

IIUC the tools are required for determining the version (which could possibly be implemented differently in the build project's GitVersionRunner?) and to format the code.
If you want, I can try messing around with both these issues.
Do give me a heads up what type of solution you had in mind :)

@Vogel612
Copy link
Contributor Author

Sidenote: I'm currently running into issues trying to replace GitVersion.CommandLine (3.6.2) as Tool-Dependency with GitVersionCommon (4.0.0) as NuGet package. The NativeBinaries from LibGit2Sharp are currently binding to curl & openssl with a version apparently deprecated for security reasons.

LibGit2Sharp is looking at replacing that native binding, but unfortunately that has not happened yet. As such I am unable to replace the GitVersion related code right now.

@ryangribble
Copy link
Contributor

I'm thinking if we had "nofullframework" mode for local use only, it could skip running gitversion as well as not build the net45 targets you've already implemented

This implies that NoFramework is intended for local use only
Accordingly a warning message has been set up in the Lifetime of the Build
@Vogel612
Copy link
Contributor Author

@ryangribble were you thinking something along these lines? Happy to make some more adjustments 👍

@ryangribble
Copy link
Contributor

Yep that's what i was thinking - love the warning you added by the way 👍

A couple of other thoughts (apologies im on mobile at the moment so I dont have links to affected source code lines etc)

  • We should skip the "ToolInstaller" of Gitversion and CodeFormatter tools when running in this mode (since it will fail if mono/nuget etc arent present)
  • The code formatter also wouldn't work on linux/mac without mono (it may even not work WITH mono, i havent tried!) so perhaps that build task should log a warning if it is ever executed in NoFramework mode rather than trying to do stuff and blowing up (assuming it does blow up!)
  • you could change default behaviour of build.sh to run in NoFramework mode by default (so things work for linux/mac local users "out of the box". But then change travis.yml so that it still builds the full framework stuff on tarvis
  • I wonder if it would be clearer to name the mode "CoreOnly" rather than "NoFramework" and invert the logic (!$NoFramework in the csproj conditions is a bit of a confusing double negative)... thoughts?

@Vogel612
Copy link
Contributor Author

We should skip the "ToolInstaller" of Gitversion and CodeFormatter tools when running in this mode (since it will fail if mono/nuget etc arent present)

Done

The code formatter also wouldn't work on linux/mac without mono (it may even not work WITH mono, i havent tried!) so perhaps that build task should log a warning if it is ever executed in NoFramework mode rather than trying to do stuff and blowing up (assuming it does blow up!)

The code formatter only runs on windows in the first place, but since we're not downloading the tools for CoreOnly builds, I added an explicit check to the ShouldRun implementation of the task.

you could change default behaviour of build.sh to run in NoFramework mode by default (so things work for linux/mac local users "out of the box". But then change travis.yml so that it still builds the full framework stuff on tarvis

travis invokes the buildscript. Changing the buildscript's default would involve creating an explicit switch to turn off the default. My bash is somewhere between bad and gruesome, so I don't want to mess around there

I wonder if it would be clearer to name the mode "CoreOnly" rather than "NoFramework" and invert the logic (!$NoFramework in the csproj conditions is a bit of a confusing double negative)... thoughts?

Agreed and done

@ryangribble
Copy link
Contributor

. My bash is somewhere between bad and gruesome, so I don't want to mess around there

I year ya 🤣

how about this though - the cake script could default CoreOnly to be true for Windows and false otherwise. Travis can pass in false which should override the default value. The bash script should already pass through any parameter provided to it (you should see some already being passed in the travis yml)

@ryangribble
Copy link
Contributor

The linksources parameter is a good example to follow. Travis passes it in, nothing special for it in the build.sh (it passes all parameters through to cake) and the cake build uses the value provided or uses a default if none specified (here https://github.com/octokit/octokit.net/blob/master/build/Lifetime.cs#L14)

We can follow similar approach except default for CoreOnly will be conditional on windows or not

My aim here is for a contributor locally on Linux or OSX just runs build.sh and everything just works (without losing anything on the Windows or travis/appveyor side)

@Vogel612
Copy link
Contributor Author

@ryangribble could you kick travis for me? It seems there has been a timeout in the LinkSources check

@ryangribble
Copy link
Contributor

There's a section in the build that explicitly logs the value of the parameters, could we add the CoreOnly one to that?

This is looking good! I'll do a test on my MacBook today 😁

@ryangribble
Copy link
Contributor

Great stuff! Tested on my macbook tonight, works perfectly 👍

Is this ready for merging, from your perspective @Vogel612

@Vogel612
Copy link
Contributor Author

Anytime :)

@Vogel612
Copy link
Contributor Author

@ryangribble poke: what's the latest on this?

@ryangribble
Copy link
Contributor

Ah sorry @Vogel612 I've been on holidays this week, I'll try to get to it today 😬

@Vogel612
Copy link
Contributor Author

@ryangribble poke: Do tell me when I start to seriously annoy you with this, otherwise I'll keep more or less regularly poking you here 😉

@ryangribble
Copy link
Contributor

It's all good... I'm doing some serious juggling at the moment so poking is fine/necessary 😀

So I was just doing some final testing and found an issue where on Windows in both VS2017 and dotnet cmdline, it now is only building the netstandard1.1 targets instead of netstandard1.1 and net45. Makes sense I guess since only Cake currently has the logic to set this flag appropriately

If I were to propose the ideal result of this PR, it would be:

✅ Cake builds on CI (appveyor and travis, all OS'es), build both platforms
✅ Cake builds on linux/mac build only netstandard1.1
✅ Cake builds on windows build both platforms
✅ local "native" builds on linux/mac build only netstandard1.1
❌ local "native" builds on windows build both platforms

So far we have 4 out of 5. Maybe we need to actually set a default value for CoreOnly in MSBuild land, by detecting if on Windows or not?

@Vogel612
Copy link
Contributor Author

Vogel612 commented Jan 30, 2019

rough quick-patch: Add <CoreOnly Condition="$(CoreOnly) == '' and $(OS) == 'Windows NT'">False</CoreOnly> to all property groups involved? Can you verify for me that this fixes the native windows build?

Alternatively <CoreOnly Condition="$(CoreOnly) == '' and $([MSBuild]::IsOsPlatform('Windows'))">False</CoreOnly> seems to be the preferred solution in the issue you linked.

@ryangribble
Copy link
Contributor

ryangribble commented Jan 30, 2019

yeah, sounds like the IsOsPlatform() is the recommended option. I tested on windows adding your 2nd option to the csproj files and it appears to work 👍

'native' msbuild invocations do not automatically set CoreOnly according to the platform
they are running on. As such we set CoreOnly to False on Windows, unless CoreOnly has been
specified already.
@ryangribble
Copy link
Contributor

Tested on windows and mac, plus reviewed the travis and appveyor builds and everything is looking great! 5 out of 5!

woohoo!

LGTM

Thanks for working on this @Vogel612

@ryangribble
Copy link
Contributor

release_notes: Adjust Cake and native build configurations to allow building on OSX/Linux out of the box

@ryangribble ryangribble changed the title Introduce 'NoFramework' switch Enable building on linux without project file changes Jan 31, 2019
@ryangribble ryangribble merged commit 5e751a6 into octokit:master Jan 31, 2019
@Vogel612 Vogel612 deleted the linux-build branch January 31, 2019 11:07
@nickfloyd nickfloyd added Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR and removed category: housekeeping labels Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build on Linux
3 participants