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

Incorrect build env created based on running app #70

Closed
ltcmelo opened this issue Aug 8, 2018 · 7 comments
Closed

Incorrect build env created based on running app #70

ltcmelo opened this issue Aug 8, 2018 · 7 comments

Comments

@ltcmelo
Copy link
Contributor

ltcmelo commented Aug 8, 2018

Hey @daveaglick, I don't agree with this logic that you have in EnvironmentFactory:

// If we're running on .NET Core, use the .NET Core SDK regardless of the project file
if (BuildEnvironment.IsRunningOnCore)
{
    return CreateCoreEnvironment(options);
}

Our tool, for instance, is built on .NET Core but we perform static analysis on .NET Framework apps as well. In our case, that check would always return true and, in most of the times, it'd be an incorrect decision (most of our customers are running .NET Framework).

I've done a few changes in our fork, but I can't put them together with the other PRs at the moment. I'm not sure what's the best approach here, but notice that the if statement above happens even before checking for a possibly specified targetFramework. What do you say?

@ltcmelo
Copy link
Contributor Author

ltcmelo commented Aug 8, 2018

Interesting... I've done further testing and, indeed, it seems that, even for a .NET Framework app, we need to configure the environment for .NET Core. I suppose this is due to the dynamic linking for the common targets.

I've experimented a few changes, by forcing certain paths of the .NET Framework, but ended up running into issues similar to the one you, @daveaglick, created yourself: #23.

Can you year your opinion on this?

ltcmelo added a commit to ShiftLeftSecurity/Buildalyzer that referenced this issue Aug 8, 2018
@daveaglick
Copy link
Collaborator

Yeah, there’s a delicate balance between the host platform, the project being build (SDK vs. non-SDK project style, .NET Framework vs. .NET Core targets), and the SDKs/VS instances available on the system at the time. I’ve tried to cover as many combinations as possible which is why there are so many integration tests. Sometimes you can get a .NET Core host to build a .NET Framework project or vice versa, and Buildalyzer has logic to try and figure out when that’ll work and when it won’t. However, there are some pairings that simply aren’t going to work. For example, the .NET Core SDK from a .NET Core host is never going to be able to compile a UWP project because those MSBuild targets only target .NET Framework and only exist in the VS MSBuild installation when the UWP targetting pack is installed.

@ltcmelo
Copy link
Contributor Author

ltcmelo commented Aug 25, 2018

Thanks for the feedback and sorry for the delay @daveaglick. I've done some amount of experimentation making our static analyzer correctly build target apps that are written in different frameworks/platforms. Let me share a few thoughts.

Originally, I implemented the entire analyzer in .NET Core and the intention was that it would be able to build target apps (i.e., those to be analyzed) that are written either in .NET Core or .NET Framework. To this task, the plan was to use your great library, Buildalyzer. But this only worked partially...

Buildalyzer, by default, attempts to use the environment of .NET Core even when the target app is a .NET Framework one. While that works in a handful of cases, it fails in the majority of .NET Framework projects I tested it on, with varying versions of .NET (between 4.0 and 4.7) and Visual Studio, and with different sorts of projects (e.g., webapi, asp, etc).

Even though I played a bit with the MSBuild setup code in Buildalyzer, I typically ended up with missing targets or with incompatibilities arriving from the mix of .NET Core and .NET Framework runtime assemblies. Some of those cases could be solved by copying a file from a place to the another (where it was expected). But there are still cases in which certain "assumptions" are made by MSBuild and no properties seem to exist so that such behavior can be fine tuned. The results was some inconsistency being injected into the build or failed to be found.

Right now, I've modularized our analyzer and we have a version for .NET Core and another for .NET Framework. I continue to use Buildalyzer for .NET Core but I moved to MSBuildWorkspace for .NET Framework. BTW, MSBuildWorkspace has its own limitations, since I still need to either package certain MSBuild assemblies likeMicrosoft.Build.Framework (just as JetBrains is doing) or require a matching version of MSBuild in the target machine - this latter situation isn't solved by MSBuildLocator.

I believe that it's still worth to give it a try of making a .NET Core-built Buildalyzer to robustly build .NET Framework apps, but, to this end, I'd suggest that you move away from the strategy of picking the .NET Core environment just because the analyzer is running on .NET Core - as I mentioned in the original post. I did read your post where you mention to "use the .NET Core SDK if you're running MSBuild from a .NET Core runtime, even if the project you're compiling targets .NET Framework", but my experimentations and discoveries made through in a few github issues (some of them created by you) tell me this is not the way to go. Perhaps, this is something to dig further and to submit according PRs to MSBuild's project until everything can be customized by means of MSBuild properties.

@daveaglick
Copy link
Collaborator

Thanks for the detailed summary of where you’re at. It’s very helpful. To be honest, I’m not invested in any particular method or combination of host environment, build tools, etc. Buildalyzer has ended up working the way it does almost entirely due to reverse engineering and trial and error.

Buildalyzer, by default, attempts to use the infrastructure of .NET Core even when the target app is a .NET Framework one. While that works in a handful of cases, it fails in the majority of .NET Framework projects I tested it on, with varying versions of .NET (between 4.0 and 4.7) and Visual Studio, and with different sorts of projects (e.g., webapi, asp, etc).

I’d be interested to know more about the kinds of projects you were building and the host environment in those cases where it failed. I’ve added a pretty comprehensive set of open source projects as integration tests and attempt to build them from both .NET Framework and .NET Core hosts. Some of them fail, but in expected ways (I.e., portable project from .NET Core where the .NET Core MSBuild targets don’t understand portable projects). My understanding was that this covered a lot of ground, but your experience suggests otherwise. I’d love to know if the integration tests need to be expanded and in what way.

@ltcmelo
Copy link
Contributor Author

ltcmelo commented Aug 27, 2018

I’ve added a pretty comprehensive set of open source projects as integration tests and attempt to build them from both .NET Framework

Do you run those tests on a clean environment? Because, as I said, some of the issues that I faced could be solved by manually copying files that would have been seen by a .NET Framework MSBuild to a place where they can be seen in a .NET Core build. Any chance your test system is "polluted" with such changes?

As an example, a basic case that does not work for me out of the box is to (i) create a .NET Framework WebApi through Visual Studio's wizard; (ii) ensure that it compiles from the command line with msbuild; and (iii) try to build it with a .NET Core-built Buildalyzer.

@daveaglick
Copy link
Collaborator

Do you run those tests on a clean environment?

For the most part, yes. Both the .NET Framework and the .NET Core set of integration tests get run on AppVeyor CI which launches a fresh environment for each build. Those tests create a matrix of sorts with each OSS test project being built by Buildalyzer both from a .NET Framework host and a .NET Core host.

The next step to investigate what you're seeing is to get a repro I can dig into. I haven't been able to yet, but I'll keep trying. Maybe there's something about specific project types or dependencies like the WebApi example you gave that trigger what you're experiencing.

@daveaglick
Copy link
Collaborator

I just published Buildalyzer 2.0 which runs MSBuild out-of-process (as opposed to the old way of running it via MSBuild APIs). In general, if you can build it Buildalyzer should now be able to build it. Given how drastically different this new technique is, I'm closing all existing bug reports.

Please try the new version and if you're still having problems, please open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants