eclipse / omr

Eclipse OMR™ Cross platform components for building reliable, high performance language runtimes
http://www.eclipse.org/omr
Other
940 stars 394 forks source link

Extend OMR PR Testing Cmdline #6477

Open babsingh opened 2 years ago

babsingh commented 2 years ago

Recently, there have been PRs where the code is disabled by default via preprocessor flags:

The PR builds do not have an option to enable preprocessor flags, which prevents such changes to be tested. Example: jenkins build cflags="-DDUMP_DBG" cxxflags="-DDUMP_DBG" all.

Based upon the discussion in this week's OMR architecture meeting (https://github.com/eclipse/omr/issues/6416), there are TWO OPTIONS:

  1. Aliases. e.g. jenkins build all mode=debug.
  2. Pass pre-processor flags via cmd line option. e.g. jenkins build all cflags+="-DDUMP_DBG" cxxflags-="-DDUMP_DBG".

[Option 1] Aliases pros:

[Option 1] Aliases cons:

[Option 2] Cmdline option pros:

[Option 2] Cmdline option cons:

Security (e.g. execution of malicious code via injection) is not a major concern because PR builds can only be launched by a select trusted individuals.

babsingh commented 2 years ago

fyi @0xdaryl @dsouzai @mstoodle @AdamBrousseau

Issue opened for further discussion.

dsouzai commented 2 years ago

(Continuing discussion from the meeting) I suppose it probably does make sense to just have the build flags explicitly passed in for now, and then after some period of time (say a year or something like that) we can create aliases based on the most commonly used flags for convenience (if applicable). At the moment we'd just be guessing as to what the aliases should be. I do think the aliases are a good idea from a human factors pov, but it should also be data driven.

For the sake of having a standard bar of acceptance though, I do think what @mstoodle said should be enforced, namely, even if builds launched with custom flag(s) pass, the PR should only be merged if builds without the custom flag(s) also pass.