Skip to content

Conversation

@gildor
Copy link
Contributor

@gildor gildor commented Jan 9, 2018

This is mostly proposal for change, I refactored and simplified build.gradle of the project.
But there is also change about kscript.jar.
Main idea: do not copy kscript jar file to root of the project and use the default build dir instead, so we have simple build script and use Gradle convention.

I've updated shadow plugin (version 2.0.2 with bugfixes. 2.0.1 not available on Gradle Plugins Portal).

But I'm not that I miss nothing, please review and comment what do you think about those changes

  • Main build.gradle refactored:
    • Use plugins dsl instead of buildscript
    • Shadow JAR file name now consistent for all versions and we avoid copy to root of the project
    • Fixed project version
    • Cleaned comments
  • .gitignore cleanup from useless ignores
  • Scripts migrated to new kscript.jar location build/libs/kscript.jar
  • Junit 4.12 (the latest bug fix release of Junit)
  • Because Travis CI doesn't cache anything switched to Gradle Wrapper bin distribution to speed up Gradle downloading

@gildor gildor force-pushed the build-gradle-refactoring branch 5 times, most recently from 3eb2177 to d020c77 Compare January 9, 2018 06:18
@gildor
Copy link
Contributor Author

gildor commented Jan 9, 2018

Fixed tests and rebased

@holgerbrandl
Copy link
Collaborator

That's a lot of stuff. Very impressive.

  • I definitely like a more modern build.gradle.
  • No need to patch anything in experimental. I just kept it there for archival reasons
  • Do we really need the actual version in the build.gradle? It seems never used. Couldn't we use a dummy placeholder instead to keep it happy? Otherwise we have to change another file with each release.
  • Why do we need to keep assert.sh under version control? If at all, I'd rather put it under test to keep the repo root as clean as possible
  • test_script.kts should not be under git control (and was not before I think)

What I'm not yet so convinced about is new location of the jar. Before the binary installation had a very simplistic layout (one dir with 2 files). Now the 2 files are scattered over a directory hierarchy without any obvious need (except maybe gradle convention).

@gildor
Copy link
Contributor Author

gildor commented Jan 9, 2018

Couldn't we use a dummy placeholder instead to keep it happy

Yes, definitely. I recommend just remove version. I keep it because at least real version is better than 0.1

Will revert changes in experimental

Sorry, my fault, accidentally add assert.sh and test_script.kts. I run tests locally and they generate those files
Will fix it and add to .gitignore

Before the binary installation had a very simplistic layout

It's not indented to change for the end user, only for local testing. I will check binaries again and will keep current layout for them.
The main idea to keep all build only files in build dir and do not affect build configs.

@holgerbrandl
Copy link
Collaborator

To keep the installation layout as it was, gradle could copy kscript (the shell script) into the build/libs, and we run it from there for the tests (seems fine with me). Otherwise we'd need a separate launcher for the tests (which is not good imho).

@gildor
Copy link
Contributor Author

gildor commented Jan 9, 2018

@holgerbrandl yes, exactly, I'm already fixing this

@holgerbrandl
Copy link
Collaborator

Hey @gildor , any estimate on how long it will take to fixing those remaining bits? I'm just curious because I may want to merge pr #93 somewhen soon, and it may conflict with some of your changes.

@gildor
Copy link
Contributor Author

gildor commented Jan 29, 2018

Hi @holgerbrandl. Sorry, been really busy last couple weeks.
Be free to merge #93 and I will update PR with fixes and resolved conflicts after merge

- Main build.gradle refactored:
    - build.gradle converted to gradle/kotlin-dsl (yes, Kotlin scripting for build config of Kscript)
    - Use plugins dsl instead of buildscript
    - Shadow JAR file name now consistent for all versions and we avoid copy to root of the project
    - Fixed project version
    - Cleaned comments
- .gitignore cleanup from useless ignores
- Default lifecycle task assemble now builds shadowJar instead of jar
- Junit 4.12 (the latest bug fix release of Junit)

# Conflicts:
#	.travis.yml
#	build.gradle
#	src/main/kotlin/kscript/app/ResolveIncludes.kt
#	test/test_suite.sh
@gildor gildor force-pushed the build-gradle-refactoring branch from d020c77 to 018c63a Compare April 30, 2018 17:59
@gildor
Copy link
Contributor Author

gildor commented Apr 30, 2018

Time to fix this old pull request...

Removed accidentally committed test files
Version removed from gradle config
kscript moved to build/libs, I fixed also some configs, please check. Also kscript now in src, to prevent contributors to run ./kscript from source dir
Additionally, I converted build.gradle to kotlin-dsl, if you think that we don't need it, please message, I will return groovy, but now we use Kotlin Scripting in build file %)

I would like also improve test script, and avoid creation of temporary files in project dir, but it seems not so easy to do, probably I will create an issue with a discussion about test suite

Looks like --text opt config is broken, please check this PR, otherwise, some tests fail

@holgerbrandl holgerbrandl merged commit 018c63a into kscripting:master May 4, 2018
@holgerbrandl
Copy link
Collaborator

Great PR. Thanks a lot. Especially the refac of build.gradle into build.gradle.kts.

I took the liberty to inline KSCRIPT_BUILD_LIBS which typically had just one or at max two usage sites, and was not yet defined in the local test instructions.

@gildor
Copy link
Contributor Author

gildor commented May 4, 2018

@holgerbrandl Okay, sure!

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

Successfully merging this pull request may close these issues.

2 participants