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

test: update build.gradle and junit test #3

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

pollend
Copy link
Member

@pollend pollend commented Feb 13, 2021

No description provided.

Copy link

@skaldarnar skaldarnar left a comment

Choose a reason for hiding this comment

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

I've left a comment on the build.gradle. Maybe @Cervator knows whether modifying that file is okay, or whether we could remove it altogether? I just vaguely remember that Kallisti is a somewhat special module...

sourceSets = [project.sourceSets.main]
header = project.file("docs/NOTICE")
ignoreFailures = true
}

Choose a reason for hiding this comment

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

Should we keep this? what exactly does it do? Removing some license mangling sounds weird...

Copy link
Member

Choose a reason for hiding this comment

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

There are indeed some special license considerations for this when in the context of the standalone Kallisti project, not the Terasology module :-)

@DarkWeird
Copy link

I've left a comment on the build.gradle. Maybe @Cervator knows whether modifying that file is okay, or whether we could remove it altogether? I just vaguely remember that Kallisti is a somewhat special module...

Yeah. This is special module. It using native.
JNLua. We haven't special algo for natives from modules. And because cannot remove build gradle file there.
P.S. ModularComputer using Lua too.. but pure java version

@Cervator
Copy link
Member

More answer: the reason Kallisti has a custom build.gradle is because it is built both as a Terasology module and as an independent library that asie can use for other projects.

The Terasology module build completely ignores this file and relies on the standard one copied in by our automation

The standalone build does use this file since it needs to be able to find JNLua and other dependencies, which in the Terasology case are provided by the engine

It ends up looking weird in a Terasology workspace since the build.gradle is overwritten when you groovyw module get Kallisti so it shows as changed.

As a consequence this file should not be modified to follow any Terasology changes, but may indeed separately be updated just in general to track the independent Kallisti project. Shouldn't merge this as-is, but I'm not sure what the impact would be to JOMLify it since there is at least one external user

Copy link
Member

@Cervator Cervator left a comment

Choose a reason for hiding this comment

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

See other bigger comment: In short we need to coordinate with @asiekierka as this repo builds two different things. The build.gradle should remain customized for the standalone Kallisti build with no relevance to the Terasology module.

The exact impact from JOMLifying this or changing how unit tests work ... might need some thinking and poking around. Or splitting the repo so we have a plain module and a separate library.

Cannot merge as-is

url "http://artifactory.terasology.org/artifactory/virtual-repo-live"
}
plugins {
id("terasology-module")
Copy link
Member

Choose a reason for hiding this comment

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

Negative - this build file does not serve as a Terasology module. It gets that from Jenkins / other automation. This build file should let it build entirely independently - also the reason why the repo has a Gradle wrapper etc :-)

Copy link
Member Author

@pollend pollend Feb 13, 2021

Choose a reason for hiding this comment

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

all we need is the kallist dependency right :?

Copy link
Member

Choose a reason for hiding this comment

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

Come to think of it - we have a Kallisti both here under the Terasology org and under MovingBlocks. So we could just reduce this one to module-level (no build.gradle at all, uses JOML) and leave the other repo as-is in case of ever wanting to use Kallisti as a lib, but otherwise not mind it until then

sourceSets = [project.sourceSets.main]
header = project.file("docs/NOTICE")
ignoreFailures = true
}
Copy link
Member

Choose a reason for hiding this comment

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

There are indeed some special license considerations for this when in the context of the standalone Kallisti project, not the Terasology module :-)

@asiekierka
Copy link

I haven't used Kallisti in a long time and the projects I wanted to use it in have been abandoned. Feel free to cut that out of the picture, if you want to.

@Cervator
Copy link
Member

I haven't used Kallisti in a long time and the projects I wanted to use it in have been abandoned. Feel free to cut that out of the picture, if you want to.

Aw, that's a pity to hear :-( Still, appreciate the quick response, hope all is otherwise well 👍

@asiekierka
Copy link

I wouldn't worry about it - a project I'm not really involved in shouldn't have to cater to my past wishes :-) If it makes life easier for you...

Other than that, I'm fine, yeah.

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