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

Enhancement request - do not assume 'obj' folder name #699

Open
msherms2 opened this issue May 9, 2023 · 15 comments
Open

Enhancement request - do not assume 'obj' folder name #699

msherms2 opened this issue May 9, 2023 · 15 comments
Milestone

Comments

@msherms2
Copy link

msherms2 commented May 9, 2023

return Path.Combine(baseIntermediateOutputPath, "obj", folderName);

This code assumes the directory specified using the -biop flag is the parent of the obj folder. However, MSBuild provides you the ability to rename object folders to another name (such as .obj). Assuming the folder is always <parent>/obj/<subfolder> seems unnecessary. To avoid a breaking change here, you might be able to check if that path exists, and then use it, but otherwise try just directly using the directory specified with -biop.

To get around this, we are manually copying our intermediate directory to another location (<directory>/obj) and then passing <directory> to -biop flag.

@Bertk
Copy link
Contributor

Bertk commented May 12, 2023

Could you please clarify why you use a different name than ˋobjˋ?
This is the default name for C#, C++, … and well known to developers.

BaseIntermediateOutputPath All The top-level folder where all configuration-specific intermediate output folders are created. The default value is obj. The following code is an example: c:\xyz\obj</BaseIntermediateOutputPath>

see also :

@msherms2
Copy link
Author

Could you please clarify why you use a different name than ˋobjˋ? This is the default name for C#, C++, … and well known to developers.

BaseIntermediateOutputPath All The top-level folder where all configuration-specific intermediate output folders are created. The default value is obj. The following code is an example: c:\xyz\obj
see also :

Why? I don’t suspect I have a good answer – it wasn’t even me that did it. But given that Microsoft lets you change this, it might be better to just relax the logic and require the entire path for CycloneDX. I don't see any gain from adding a magic folder named “obj”. It actually made it a little more annoying for us, because we have to figure out where the obj folder is, and then pass in the parent folder. :)

thanks,
Matt

@mtsfoni
Copy link
Contributor

mtsfoni commented Dec 28, 2023

I think I actually saw code in another class that checks the proj file for the obj-output folder.

This shouldn't be a big deal, i'll try to push it in.

@mtsfoni mtsfoni added this to the v3.1.0 milestone Dec 28, 2023
@mtsfoni
Copy link
Contributor

mtsfoni commented Dec 28, 2023

So i was able to reproduce it, by setting Project/PropertyGroup/BaseIntermediateOutputPath for example to intermediate\ then everything that usually goes into the obj-folder goes into intermediate.

Interestingly, even though there is a project property, setting it does not transfer everything, but only the content of obj\Debug. It must apparently be set in the Directory.Build.props.

My plan is to deprecate the current -biop / --BaseIntermediateOutputPath and add a new one, that doesn't assume the obj.

@Bertk
Copy link
Contributor

Bertk commented Dec 29, 2023

@mtsfoni Please double-check the usage of BaseIntermediateOutputPath and new artifacts properties introduced with .NET 8.0 "Simplified output path".

  • UseArtifactsIntermediateOutput
  • UseArtifactsOutput
  • ArtifactsBinOutputName
  • ArtifactsPackageOutputName
  • ArtifactsPath
  • ArtifactsPivots
  • ArtifactsProjectName
  • ArtifactsPublishOutputName

In my understanding this enhancement should be aligned with the .NET SDK approach and not possible MSBuild behavior.

C:\Program Files\dotnet\sdk\8.0.100\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.DefaultArtifactsPath.props

<!--
***********************************************************************************************
Microsoft.NET.DefaultArtifactsPath.props

WARNING:  DO NOT MODIFY this file unless you are knowledgeable about MSBuild and have
          created a backup copy.  Incorrect changes to this file will make it
          impossible to load or build your projects from the command-line or the IDE.

Copyright (c) .NET Foundation. All rights reserved.
***********************************************************************************************
-->
<Project>

  <!-- This .props file may be imported either from Sdk.props or from Microsoft.NET.DefaultOutputPaths.targets, depending
       on whether artifacts output properties were set in Directory.Build.props or not.

       Set a property to indicate it was imported, so we can avoid a duplicate import. -->
  <PropertyGroup>
    <_DefaultArtifactsPathPropsImported>true</_DefaultArtifactsPathPropsImported>
  </PropertyGroup>

  <!-- Setting ArtifactsPath automatically opts in to the artifacts output format -->
  <PropertyGroup Condition="'$(ArtifactsPath)' != '' And '$(UsingMicrosoftArtifactsSdk)' != 'true'">
    <UseArtifactsOutput Condition="'$(UseArtifactsOutput)' == ''">true</UseArtifactsOutput>
    <IncludeProjectNameInArtifactsPaths Condition="'$(IncludeProjectNameInArtifactsPaths)' == ''">true</IncludeProjectNameInArtifactsPaths>
    <_ArtifactsPathLocationType>ExplicitlySpecified</_ArtifactsPathLocationType>
  </PropertyGroup>

  <!-- Set up base output folders if UseArtifactsOutput is set -->
  <PropertyGroup Condition="'$(UseArtifactsOutput)' == 'true' And '$(ArtifactsPath)' == '' And '$(_DirectoryBuildPropsBasePath)' != ''">
    <!-- Default ArtifactsPath to be in the directory where Directory.Build.props is found
         Note that we do not append a backslash to the ArtifactsPath as we do with most paths, because it may be a global property passed in on the command-line which we can't easily change -->
    <ArtifactsPath>$(_DirectoryBuildPropsBasePath)\artifacts</ArtifactsPath>
    <IncludeProjectNameInArtifactsPaths Condition="'$(IncludeProjectNameInArtifactsPaths)' == ''">true</IncludeProjectNameInArtifactsPaths>
    <_ArtifactsPathLocationType>DirectoryBuildPropsFolder</_ArtifactsPathLocationType>
  </PropertyGroup>

  <PropertyGroup Condition="'$(UseArtifactsOutput)' == 'true' And '$(ArtifactsPath)' == ''">
    <!-- If there was no Directory.Build.props file, then put the artifacts path in the project folder -->
    <ArtifactsPath>$(MSBuildProjectDirectory)\artifacts</ArtifactsPath>
    <_ArtifactsPathLocationType>ProjectFolder</_ArtifactsPathLocationType>
  </PropertyGroup>
</Project>

SDKs which are not shipped with .NET

@msherms2
Copy link
Author

msherms2 commented Jan 2, 2024

@mtsfoni Please double-check the usage of BaseIntermediateOutputPath and new artifacts properties introduced with .NET 8.0 "Simplified output path".

  • UseArtifactsIntermediateOutput
  • UseArtifactsOutput
  • ArtifactsBinOutputName
  • ArtifactsPackageOutputName
  • ArtifactsPath
  • ArtifactsPivots
  • ArtifactsProjectName
  • ArtifactsPublishOutputName

In my understanding this enhancement should be aligned with the .NET SDK approach and not possible MSBuild behavior.

C:\Program Files\dotnet\sdk\8.0.100\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.DefaultArtifactsPath.props

<!--
***********************************************************************************************
Microsoft.NET.DefaultArtifactsPath.props

WARNING:  DO NOT MODIFY this file unless you are knowledgeable about MSBuild and have
          created a backup copy.  Incorrect changes to this file will make it
          impossible to load or build your projects from the command-line or the IDE.

Copyright (c) .NET Foundation. All rights reserved.
***********************************************************************************************
-->
<Project>

  <!-- This .props file may be imported either from Sdk.props or from Microsoft.NET.DefaultOutputPaths.targets, depending
       on whether artifacts output properties were set in Directory.Build.props or not.

       Set a property to indicate it was imported, so we can avoid a duplicate import. -->
  <PropertyGroup>
    <_DefaultArtifactsPathPropsImported>true</_DefaultArtifactsPathPropsImported>
  </PropertyGroup>

  <!-- Setting ArtifactsPath automatically opts in to the artifacts output format -->
  <PropertyGroup Condition="'$(ArtifactsPath)' != '' And '$(UsingMicrosoftArtifactsSdk)' != 'true'">
    <UseArtifactsOutput Condition="'$(UseArtifactsOutput)' == ''">true</UseArtifactsOutput>
    <IncludeProjectNameInArtifactsPaths Condition="'$(IncludeProjectNameInArtifactsPaths)' == ''">true</IncludeProjectNameInArtifactsPaths>
    <_ArtifactsPathLocationType>ExplicitlySpecified</_ArtifactsPathLocationType>
  </PropertyGroup>

  <!-- Set up base output folders if UseArtifactsOutput is set -->
  <PropertyGroup Condition="'$(UseArtifactsOutput)' == 'true' And '$(ArtifactsPath)' == '' And '$(_DirectoryBuildPropsBasePath)' != ''">
    <!-- Default ArtifactsPath to be in the directory where Directory.Build.props is found
         Note that we do not append a backslash to the ArtifactsPath as we do with most paths, because it may be a global property passed in on the command-line which we can't easily change -->
    <ArtifactsPath>$(_DirectoryBuildPropsBasePath)\artifacts</ArtifactsPath>
    <IncludeProjectNameInArtifactsPaths Condition="'$(IncludeProjectNameInArtifactsPaths)' == ''">true</IncludeProjectNameInArtifactsPaths>
    <_ArtifactsPathLocationType>DirectoryBuildPropsFolder</_ArtifactsPathLocationType>
  </PropertyGroup>

  <PropertyGroup Condition="'$(UseArtifactsOutput)' == 'true' And '$(ArtifactsPath)' == ''">
    <!-- If there was no Directory.Build.props file, then put the artifacts path in the project folder -->
    <ArtifactsPath>$(MSBuildProjectDirectory)\artifacts</ArtifactsPath>
    <_ArtifactsPathLocationType>ProjectFolder</_ArtifactsPathLocationType>
  </PropertyGroup>
</Project>

SDKs which are not shipped with .NET

I would say that while this is true, my original point is still valid. I don't care what you set valid microsoft properties to - the code guessing that a folder named Obj should be used isn't valid if you are allowed to rename what the obj output folder is. I should be able to put those to D:\Cheese\ if I feel like it, and the tool should just allow me to specify that as a path. The proposed change sounds desirable, and I don't think there's anything that would need to be done to account for specific language constructs. Thoughts?

@mtsfoni
Copy link
Contributor

mtsfoni commented Jan 2, 2024

I think I'd prefer to stay stupid for now.
The user can tell us where his assets file is, and that is from where we get it.
Eventually we probably need to read the Directory.Builds.Probs, but this we can solve without.

What I am not sure about yet is the behaviour with --recursive. Actually, I am not even sure what happens now in the case that -biop and -rs are both set and what would actually make sense.
(I think -rs is an annoying band aid to keep downwards compatibility for packages.config)

@Bertk
Copy link
Contributor

Bertk commented Jan 3, 2024

I agree this is not a pressing topic and not requested the last 4 years. Currently the value of MSBuild property BaseIntermediateOutputPath is not available and a possible solution is to introduce Buildalyzer ( see also #736).
This will have an impact on performance but also provide access to the value of BaseIntermediateOutputPath.

@mtsfoni I never used the -rs option and think this is already obsolete. Typically legacy projects will not ask for SBOM ...

from C:\Program Files\dotnet\sdk\8.0.100\Sdks\Microsoft.NET.Sdk\Sdk\Sdk.props

  <!-- If ArtifactsPath or UseArtifactsOutput are set, then import .props to set ArtifactsPath here, so that BaseIntermediateOutputPath can be
       set in the ArtifactsPath.
       If the .props file is not imported here, it will be imported from Microsoft.NET.DefaultOutputPaths.targets, so that artifacts output
       properties can be set directly in the project file too (only in that case they won't affect the intermediate output). -->
  <Import Project="$(MSBuildThisFileDirectory)..\targets\Microsoft.NET.DefaultArtifactsPath.props"
          Condition="'$(UseArtifactsOutput)' == 'true' Or '$(ArtifactsPath)' != ''"/>

  <PropertyGroup Condition="'$(UseArtifactsOutput)' == 'true'">
    <UseArtifactsIntermediateOutput Condition="'$(UseArtifactsIntermediateOutput)' == ''">true</UseArtifactsIntermediateOutput>
    <ArtifactsProjectName Condition="'$(ArtifactsProjectName)' == ''">$(MSBuildProjectName)</ArtifactsProjectName>
  </PropertyGroup>
  
  <PropertyGroup Condition="'$(UseArtifactsOutput)' == 'true' And '$(BaseIntermediateOutputPath)' == '' And '$(UseArtifactsIntermediateOutput)' == 'true'">
    <BaseIntermediateOutputPath Condition="'$(IncludeProjectNameInArtifactsPaths)' == 'true'">$(ArtifactsPath)\obj\$(ArtifactsProjectName)\</BaseIntermediateOutputPath>
    <BaseIntermediateOutputPath Condition="'$(BaseIntermediateOutputPath)' == ''">$(ArtifactsPath)\obj\</BaseIntermediateOutputPath>
  </PropertyGroup>

from C:\Program Files\dotnet\sdk\8.0.100\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.DefaultOutputPaths.targets:

  <!-- If "UseArtifactsOutput" wasn't set when the MSBuild project extensions .props files were imported, then use "obj" in the project folder for the intermediate output path
         instead a folder under ArtifactsPath.  To have the intermediate output path in the artifacts folder, "UseArtifactsOutput" should be set in Directory.Build.props-->
  <PropertyGroup Condition="'$(UseArtifactsIntermediateOutput)' != 'true'">
    <BaseIntermediateOutputPath Condition="'$(BaseIntermediateOutputPath)' == ''">obj\</BaseIntermediateOutputPath>
    <BaseIntermediateOutputPath Condition="!HasTrailingSlash('$(BaseIntermediateOutputPath)')">$(BaseIntermediateOutputPath)\</BaseIntermediateOutputPath>
    <IntermediateOutputPath Condition=" $(IntermediateOutputPath) == '' ">$(BaseIntermediateOutputPath)$(_PlatformToAppendToOutputPath)$(Configuration)\</IntermediateOutputPath>
    <IntermediateOutputPath Condition="!HasTrailingSlash('$(IntermediateOutputPath)')">$(IntermediateOutputPath)\</IntermediateOutputPath>
  </PropertyGroup>

@mtsfoni
Copy link
Contributor

mtsfoni commented Jan 4, 2024

@msherms2 what about if we just implement this one:
Add support for single project.assets.json file
In that case, instead of entering the path to the project.assets.json you could just enter the assets.json directly. If the project is properly setup (-rs not necessary, no packages.config-files, call .csproj not .sln) it should be able to get the same result as a call with a .csproj.

And I don't need to build fake projects for my functional tests that mostly run via asset files.

@msherms2
Copy link
Author

msherms2 commented Jan 4, 2024

@msherms2 what about if we just implement this one: Add support for single project.assets.json file In that case, instead of entering the path to the project.assets.json you could just enter the assets.json directly. If the project is properly setup (-rs not necessary, no packages.config-files, call .csproj not .sln) it should be able to get the same result as a call with a .csproj.

And I don't need to build fake projects for my functional tests that mostly run via asset files.

I like the idea of a file instead of a object folder (I guess i now know why it needs it), but the project.assets.json files look per project, but I'm running Cyclone DX at the solution level and tracking my vulnerabilities from a component rather than project level. Would you be able to support wildcards or N files? I believe the current functionality takes one parent folder, appends obj to it (the request to change), and then does something like objFolder/**/project.assets.json. If you can do something like that in this design, I would have no problem with this approach. I didn't love the one object folder thing either but I didn't want to be too greedy.

@mtsfoni
Copy link
Contributor

mtsfoni commented Jan 5, 2024

The big problem with the call via .sln, or in this case a folder/wildcard, is always that there is not a real root-project. Every single project is just considered to be a part of the software. A few are filtered out, though (test-projects and dev-dependencies).

In my day-job, I have to do with many solutions that bundle both frontend and backend - basically two different projects, and we generate a sbom for each.

Anyway, if one would want to generate a sbom directly from multiple project.assets.json I have no information, what to put into the Metadata Component. I can read a project name and version from a single project.assets.json, but I have no way of knowing, which is the root-project (like the .exe or service). So that case could only work properly, if there is actually a metadata file imported.

@msherms2
Copy link
Author

msherms2 commented Jan 5, 2024

The big problem with the call via .sln, or in this case a folder/wildcard, is always that there is not a real root-project. Every single project is just considered to be a part of the software. A few are filtered out, though (test-projects and dev-dependencies).

In my day-job, I have to do with many solutions that bundle both frontend and backend - basically two different projects, and we generate a sbom for each.

Anyway, if one would want to generate a sbom directly from multiple project.assets.json I have no information, what to put into the Metadata Component. I can read a project name and version from a single project.assets.json, but I have no way of knowing, which is the root-project (like the .exe or service). So that case could only work properly, if there is actually a metadata file imported.

I can understand your logic there, but I'm guessing that is a problem in my current use too, where i pass it one object folder. It's got to be iterating over all json files in there somehow and combining it. For help, here's the call I'm using, I think it's relatively self explanatory (even if it may not be fully correct :) )

dotnet CycloneDX "$slnFile" --disable-package-restore --base-intermediate-output-path $biopLoc --out $outDir --set-name $slnName --set-version $compVersion --set-type Library

I think in your proposed solution this would still work as --base-intermediate-output-path somehow becomes something that can either handle N json files or N object paths.

@Bertk
Copy link
Contributor

Bertk commented Jan 5, 2024

The hard coded obj folder in ProjectFileService.cs can be replaced with the property MSBuildProjectExtensionsPath (Buildalyzer will resolve the value).
This also allows to deprecate CycloneDX tool option "-biop, --base-intermediate-output-path".

image

image

@Bertk
Copy link
Contributor

Bertk commented Jan 9, 2024

GetProjectProperty could be replaced with GetMSBuildProjectExtensionsPath and the command line option -biop should be deprecated.

        static internal string GetMSBuildProjectExtensionsPath(string projectFilePath)
        {
            var manager = new AnalyzerManager();
            try
            {
                var project = manager.GetProject(projectFilePath);
                var buildResults = project.Build();
                return buildResults.Results.SelectMany(x => x.Properties).Where(x => x.Key.Equals("MSBuildProjectExtensionsPath")).First().Value;
            }
            catch (ArgumentException)
            {
                // can only happen while testing (because it will be checked before this method is called)
            }
            return "";
        }

@msherms2
Copy link
Author

msherms2 commented Feb 6, 2024

This might have gotten lost but again my only point is if you provide a lookup, just take the whole path. I wasn't trying to make this integrated with the MS project functionality whatsoever. I just want to be able to say -biop 'artifacts\myobjectfolder' if i want to dumbly name my obj folder that. Some code I can't control renames it to .obj, and while messy, cleaning it up because this code presumes to append /obj` is an easy problem to solve. You can even do it with backwards compatibility if you check if a 'obj' subfolder exists, use that instead, but otherwise use what is supplied.

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

3 participants