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

Pass CMake flags through mono.proj #45026

Open
CoffeeFlux opened this issue Nov 20, 2020 · 4 comments
Open

Pass CMake flags through mono.proj #45026

CoffeeFlux opened this issue Nov 20, 2020 · 4 comments

Comments

@CoffeeFlux
Copy link
Contributor

See #44747 (comment) and #42711
cc: @lambdageek @vargaz

@CoffeeFlux CoffeeFlux added this to the 6.0.0 milestone Nov 20, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Nov 20, 2020
@am11
Copy link
Member

am11 commented Nov 21, 2020

--cmakeargs -D<some-variable-name-used-in-cmake>=<value> convenience syntax was introduced a few years back, originally for port work, when repos were still separate and there were too many manual steps involved to be able to build stuff by copying coreclr/corerun overlays in test, corefx and in other places (i.e. way more complex than it is today and required a lots of diskspace for port work).

Now we have multiple repos unified in the runtime repo with a concept of 'subsets'; so in theory we can enhance this syntax by adding support for subset name. Something like:

./build.sh --cmakeargs:mono -D<variable-name-used-in-cmake>=<variable-value>

but even with this manual instrumentation, we would need to agree on certain granularity: is subset enough? or should we have support for components within a subset, like coreclr_pal? to what depth (since our structure is quite hierarchical)? This kind of artificial instrumentation (in .cmd as well as .sh scripts) gets complicated fast with zero coverage for any CI use case, plus the unnecessary maintenance cost, IMO.

Problem

Out of all cmake-aware subsets, only libs break the build when unused cmake arg is passed:

# only libs is pedantic about it, other subsets do not exit with status 1 in this case
$ ./build.sh -cmakeargs -DMY_UNKNOWN_VAR=2

Proposed Solution

Note: all the 'important' cmake properties are already hard written in cmake scripts, passing --cmakeargs is essentially a convenience to override values from command-line for sake of testing. So the standard solution is to add/update the properties directly in cmake scripts (e.g. for introspection related overrides, we use cmake's documented "-C <preload-script-path>" option, see various tryrun.cmake files in this repo).

If we still believe that --cmakeargs use-cases are imperative, there are at least two simple solutions:

  1. ⚠️ [requires a simple code change] align libs subset with rest of the subsets so it does not break the build with unknown cmake arg name.
  2. ✅ [does not require any code change] control the situation from command-line by passing additional cmake(1)'s flag --no-warn-unused-cli:
    # everything just builds without code modifications
    $ ./build.sh --cmakeargs -DMY_UNKNOWN_VAR=2 --cmakeargs --no-warn-unused-cli

#2 is what I have used in past (for PAL test suite build + the rest of the repo in single shot before clr.paltests subset was introduced) and I suggest we should just document it.

cc @janvorli, @jkoritzinsky

@lambdageek
Copy link
Member

@am11 At least for #44747 I was suggesting adding an MSBuild property that could be set from the commandline (eg ./build.sh .... /p:MonoWarningsAsErrors=true) to set a specific cmake argument.

I wonder if the more general case could be covered by agreeing on some common MSBuild property vocabulary for CMake. So for example /p:CMakeArgs=-DFOO=BAR (every subset) /p:CoreClrCMakeArgs=-DFOO=BAR (Just CoreCLR), /p:RuntimeCMakeArgs=-DFOO=BAR (Mono and CoreCLR), etc.

@am11
Copy link
Member

am11 commented Nov 24, 2020

@lambdageek, the prevalent/preferred way of working, so far, has been to burn all supported properties in cmake scripts and if there are varying values of same properties, define that set of properties as a feature, e.g. FEATURE_PERFTRACING. For quick / one-off experiments, most folks change the cmake scripts directly (hint: majority user-base is using GUI tools). If it is something that multiple folks are interested in (or an interesting CI scenario), the support is still added to cmake scripts directly and wired via some subset. e.g. before ./build.sh -s clr.paltests subset was introduced, few people used ./build.sh clr -cmakeargs -DCLR_CMAKE_BUILD_TESTS=1, while most were modifying the cmake file directly: set(CLR_CMAKE_BUILD_TESTS 1).

/p:CMakeArgs=-DFOO=BAR

This is nice, though we already have this support for build.sh --cmakeargs -DFOO=BAR to msbuild, with a bit of corner case handling like when BAR is actually mysteriously quoted "BAR B'A'Z" etc.:

runtime/eng/build.sh

Lines 459 to 463 in d73c65e

# URL-encode space (%20) to avoid quoting issues until the msbuild call in /eng/common/tools.sh.
# In *proj files (XML docs), URL-encoded string are rendered in their decoded form.
cmakeargs="${cmakeargs// /%20}"
arguments="$arguments /p:TargetArchitecture=$arch /p:BuildArchitecture=$hostArch"
arguments="$arguments /p:CMakeArgs=\"$cmakeargs\" $extraargs"

Making these as a 'standard solution' -- while we have existing / competing concepts of subset as well as features -- would make things harder to martian in a long run. (I am not a maintainer, of course, so it is just my opinion based on a little bit of experience with coreclr 🙂)

@SamMonoRT SamMonoRT removed the untriaged New issue has not been triaged by the area owner label Jun 23, 2021
@steveisok
Copy link
Member

@lambdageek Is this something we are going to do for 6.0?

@lambdageek lambdageek modified the milestones: 6.0.0, 7.0.0 Jul 1, 2021
@steveisok steveisok modified the milestones: 7.0.0, Future Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants