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

Add mixed platforms #2087

Merged
merged 1 commit into from
Dec 7, 2020
Merged

Add mixed platforms #2087

merged 1 commit into from
Dec 7, 2020

Conversation

keithc-ca
Copy link
Contributor

@llxia
Copy link
Contributor

llxia commented Dec 4, 2020

Thanks @keithc-ca
Just a note, ottawa.csv also needs to be updated to support the newly added platforms. Otherwise, we may have tests running with incorrect modes. I am ok with merge this change and update ottawa.csv in the future PR.

'arm_linux' : [
'SPEC' : 'linux_arm',
'LABEL' : 'ci.role.test&&sw.os.linux&&hw.arch.aarch32',
'LABEL' : 'ci.role.test&&sw.os.linux&&hw.arch.aarch32'
Copy link
Contributor

@llxia llxia Dec 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to keep the trailing comma. This was added purposely

  • if there is a new entry, this line will not change
  • it prevents the error in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not legal in json and looking at https://groovy-lang.org/syntax.html, there are no examples of lists with a comma after the last element. Apparently groovy made an exception, but I suggest that's a bad idea (and the practice wasn't applied consistently - there's no comma after the last element of PLATFORM_MAP).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving trailing commas simplifies the git history. If we merge this PR, all of those lines will be updated, and when we look for one line history, we will find this PR and not the reason for the line in the first place.
Please see the article https://time2hack.com/are-you-using-trailing-commas-in-your-javascript/
The git commits are much cleaner if we add an extra comma.
I understand that this is groovy code and not JavaScript, but the same logic applies.

Signed-off-by: Keith W. Campbell <keithc@ca.ibm.com>
@keithc-ca
Copy link
Contributor Author

Commas added and rebased.

@llxia
Copy link
Contributor

llxia commented Dec 7, 2020

Thanks @keithc-ca
Internal Grinder_TKG/1003/ on xlinux

Copy link
Contributor

@llxia llxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@llxia llxia merged commit aa62f38 into adoptium:master Dec 7, 2020
@keithc-ca keithc-ca deleted the mixed branch December 7, 2020 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants