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

cljc version of math.combinatorics #3

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@jurgenphotek

jurgenphotek commented Feb 15, 2016

this is not actually ready yet. but would you be interested?

This would be 1.7+ only.

@Engelberg

This comment has been minimized.

Show comment
Hide comment
@Engelberg

Engelberg Feb 15, 2016

Contributor
  1. Contrib projects can't accept pull requests, but require a contributor agreement on file, and a patch filed through the JIRA system.
  2. This is a question that a lot of the contrib projects are wrestling with, because cljc is nice to have, but kills backwards compatibility. I'd like to see a couple other contrib projects go first in terms of making the switch, but in principle, I'd like to see this happen. I think this should probably port to cljc rather trivially.
Contributor

Engelberg commented Feb 15, 2016

  1. Contrib projects can't accept pull requests, but require a contributor agreement on file, and a patch filed through the JIRA system.
  2. This is a question that a lot of the contrib projects are wrestling with, because cljc is nice to have, but kills backwards compatibility. I'd like to see a couple other contrib projects go first in terms of making the switch, but in principle, I'd like to see this happen. I think this should probably port to cljc rather trivially.
@puredanger

This comment has been minimized.

Show comment
Hide comment
@puredanger

puredanger Feb 16, 2016

Contributor

Please note that currently Clojure contrib projects with cljc cannot be built in the Clojure CI system (due to a stack of yaks). The only project currently using cljc is test.check, which must be built through a painful manual process. Because of that, I would prefer to delay this until that issue is fixed.

We have mapped out the path to fixing this issue but it's been delayed due to other work. It is considered an important issue to fix and we do intend to do it.

Contributor

puredanger commented Feb 16, 2016

Please note that currently Clojure contrib projects with cljc cannot be built in the Clojure CI system (due to a stack of yaks). The only project currently using cljc is test.check, which must be built through a painful manual process. Because of that, I would prefer to delay this until that issue is fixed.

We have mapped out the path to fixing this issue but it's been delayed due to other work. It is considered an important issue to fix and we do intend to do it.

@thedavidmeister

This comment has been minimized.

Show comment
Hide comment
@thedavidmeister

thedavidmeister Mar 14, 2016

this sounds like a good idea :)

thedavidmeister commented Mar 14, 2016

this sounds like a good idea :)

@viebel

This comment has been minimized.

Show comment
Hide comment
@viebel

viebel Sep 15, 2016

@puredanger Is it possible now to have Clojure contrib projects with cljc?

viebel commented Sep 15, 2016

@puredanger Is it possible now to have Clojure contrib projects with cljc?

(:refer-clojure :exclude [update]))
clojure.math.combinatorics
(:refer-clojure :exclude [update])
#?(:cljs (:require [goog.string :refer [format]])))

This comment has been minimized.

@viebel

viebel Sep 15, 2016

@jurgenphotek the correct way of using goog.string.format is by requiring goog.string.format like this: (:require goog.string.format)

@viebel

viebel Sep 15, 2016

@jurgenphotek the correct way of using goog.string.format is by requiring goog.string.format like this: (:require goog.string.format)

@viebel

This comment has been minimized.

Show comment
Hide comment
@viebel

viebel Dec 6, 2016

Now, we can have contrib projects withcljc https://groups.google.com/forum/#!topic/clojure-dev/cwxzA3K-P3k

@jurgenphotek @Engelberg do you want to update the PR?

viebel commented Dec 6, 2016

Now, we can have contrib projects withcljc https://groups.google.com/forum/#!topic/clojure-dev/cwxzA3K-P3k

@jurgenphotek @Engelberg do you want to update the PR?

@seancorfield

This comment has been minimized.

Show comment
Hide comment
@seancorfield

seancorfield Dec 6, 2016

Member

@viebel Note the earlier comment here that Contrib projects cannot accept PRs -- changes must go through JIRA (and be covered by a signed contributor's agreement).

Member

seancorfield commented Dec 6, 2016

@viebel Note the earlier comment here that Contrib projects cannot accept PRs -- changes must go through JIRA (and be covered by a signed contributor's agreement).

@jurgenphotek

This comment has been minimized.

Show comment
Hide comment

jurgenphotek commented Dec 13, 2016

@Engelberg

This comment has been minimized.

Show comment
Hide comment
@Engelberg

Engelberg Jan 5, 2017

Contributor
Contributor

Engelberg commented Jan 5, 2017

@puredanger

This comment has been minimized.

Show comment
Hide comment
@puredanger

puredanger Jan 5, 2017

Contributor

The first step should be that "mvn clean package" builds the correct archive on your local machine.

I believe I have made the necessary changes to the build system and the project config such that it should "just work" to then deploy on the build box as before. But when that is found to be tragically wrong, please let me know. :)

Contributor

puredanger commented Jan 5, 2017

The first step should be that "mvn clean package" builds the correct archive on your local machine.

I believe I have made the necessary changes to the build system and the project config such that it should "just work" to then deploy on the build box as before. But when that is found to be tragically wrong, please let me know. :)

@puredanger

This comment has been minimized.

Show comment
Hide comment
@puredanger

puredanger Jan 5, 2017

Contributor

Note that the standard Maven build includes support for running tests in cljc files on the JVM and compiling archives that include cljc files. It does not include any support for testing cljs.

However, data.xml has done some additional work to make cljs testing happen with Nashorn in the context of the Maven build if you're interested in borrowing from there.

Contributor

puredanger commented Jan 5, 2017

Note that the standard Maven build includes support for running tests in cljc files on the JVM and compiling archives that include cljc files. It does not include any support for testing cljs.

However, data.xml has done some additional work to make cljs testing happen with Nashorn in the context of the Maven build if you're interested in borrowing from there.

@Engelberg

This comment has been minimized.

Show comment
Hide comment
@Engelberg

Engelberg Jan 6, 2017

Contributor
Contributor

Engelberg commented Jan 6, 2017

@Engelberg

This comment has been minimized.

Show comment
Hide comment
@Engelberg

Engelberg Jan 6, 2017

Contributor
Contributor

Engelberg commented Jan 6, 2017

@Engelberg

This comment has been minimized.

Show comment
Hide comment
@Engelberg

Engelberg Jan 6, 2017

Contributor
Contributor

Engelberg commented Jan 6, 2017

@bendlas

This comment has been minimized.

Show comment
Hide comment
@bendlas

bendlas Jan 6, 2017

The clojure.version property is the one, that the package gets compiled with, i.e. the one that will generate .class files. Lower versions won't be able to load it because of clojure's ABI changes, but the test matrix will have to be adjusted separately.

bendlas commented Jan 6, 2017

The clojure.version property is the one, that the package gets compiled with, i.e. the one that will generate .class files. Lower versions won't be able to load it because of clojure's ABI changes, but the test matrix will have to be adjusted separately.

@Engelberg

This comment has been minimized.

Show comment
Hide comment
@Engelberg

Engelberg Jan 6, 2017

Contributor
Contributor

Engelberg commented Jan 6, 2017

@bendlas

This comment has been minimized.

Show comment
Hide comment
@bendlas

bendlas Jan 6, 2017

So I might as well put 1.8.0 in the clojure.version property then. But how
do I make that adjustment to the test matrix?

As I said, lower versions of clojure won't be able to load class files generated by higher versions (the ABI is about classes and methods on a java level), so putting 1.8.0 will mean that your minimum requirement is 1.8.0

As for the test matrix, @puredanger helped adjust that for data.xml. Depending on your access level on jenkins, you might be able to do it yourself.

bendlas commented Jan 6, 2017

So I might as well put 1.8.0 in the clojure.version property then. But how
do I make that adjustment to the test matrix?

As I said, lower versions of clojure won't be able to load class files generated by higher versions (the ABI is about classes and methods on a java level), so putting 1.8.0 will mean that your minimum requirement is 1.8.0

As for the test matrix, @puredanger helped adjust that for data.xml. Depending on your access level on jenkins, you might be able to do it yourself.

@puredanger

This comment has been minimized.

Show comment
Hide comment
@puredanger

puredanger Jan 6, 2017

Contributor

FYI, you should not need cljc-maven-plugin to do a basic cljc build and I'd prefer not to include it if there is no reason to use it, as it's just more stuff to maintain.

I can adjust the test.matrix - everything on the build box is generated from a data file (https://github.com/clojure/build.ci/blob/master/ci_data.clj), so any manual adjustments there will get blown away.

Contributor

puredanger commented Jan 6, 2017

FYI, you should not need cljc-maven-plugin to do a basic cljc build and I'd prefer not to include it if there is no reason to use it, as it's just more stuff to maintain.

I can adjust the test.matrix - everything on the build box is generated from a data file (https://github.com/clojure/build.ci/blob/master/ci_data.clj), so any manual adjustments there will get blown away.

@Engelberg

This comment has been minimized.

Show comment
Hide comment
@Engelberg

Engelberg Jan 6, 2017

Contributor
Contributor

Engelberg commented Jan 6, 2017

@puredanger

This comment has been minimized.

Show comment
Hide comment
@puredanger

puredanger Jan 6, 2017

Contributor

@Engelberg I think it's ok to make Clojure 1.7 the min requirement and not use cljc-maven-plugin and that would be my preference. Users for <1.7 can still use the prior version if needed.

Can you commit stuff to a branch or put a patch somewhere so I can look at it?

Contributor

puredanger commented Jan 6, 2017

@Engelberg I think it's ok to make Clojure 1.7 the min requirement and not use cljc-maven-plugin and that would be my preference. Users for <1.7 can still use the prior version if needed.

Can you commit stuff to a branch or put a patch somewhere so I can look at it?

@puredanger

This comment has been minimized.

Show comment
Hide comment
@puredanger

puredanger Jan 6, 2017

Contributor

I just pushed changes to master that convert to cljc. For me, this builds and tests locally and produces a correct jar file. Someone should actually test it on ClojureScript though.

Contributor

puredanger commented Jan 6, 2017

I just pushed changes to master that convert to cljc. For me, this builds and tests locally and produces a correct jar file. Someone should actually test it on ClojureScript though.

@puredanger

This comment has been minimized.

Show comment
Hide comment
@puredanger

puredanger Jan 6, 2017

Contributor

This also builds and tests properly on the build box. Someone needs to test the clojurescript version before including it in a release though.

Contributor

puredanger commented Jan 6, 2017

This also builds and tests properly on the build box. Someone needs to test the clojurescript version before including it in a release though.

@bendlas

This comment has been minimized.

Show comment
Hide comment
@bendlas

bendlas Jan 6, 2017

advocating 1.8 to make sure I was
getting the highest-quality and speediest class files, but you were saying
it wouldn't be backwards compatible if I did that.

Hm, when looking at the jar file distributed through maven, there are no .class files. That means, that the failure I observed when upping clojure.version either was due to a (subsequently fixed) incompatibility in the project, or caused by the test-matrix using an AOT version (which could be fixed there). Either way clojure.version should just determine what is run by mvn test. Sorry for the confusion.

cljc-maven-plugin is meant as a stop-gap and proof-of-concept. I created a ticket for clojure-maven-plugin.

@puredanger it seems you remain unconvinced, that pre-1.7 + cljc compatibility is worth maintaining build tooling for. I have no qualms recognizing 1.7 as a minimum targed for "reasonably recent" clojure, based on recent survey numbers (in fact I could rewrite a couple of expressions in data.xml in a much more elegant way). If that's the message we want to send though, why even run test matrices for older versions? EDIT do you dislike including a separate maven plugin, or is it about the approach itself? i.e. does the ticket help?

bendlas commented Jan 6, 2017

advocating 1.8 to make sure I was
getting the highest-quality and speediest class files, but you were saying
it wouldn't be backwards compatible if I did that.

Hm, when looking at the jar file distributed through maven, there are no .class files. That means, that the failure I observed when upping clojure.version either was due to a (subsequently fixed) incompatibility in the project, or caused by the test-matrix using an AOT version (which could be fixed there). Either way clojure.version should just determine what is run by mvn test. Sorry for the confusion.

cljc-maven-plugin is meant as a stop-gap and proof-of-concept. I created a ticket for clojure-maven-plugin.

@puredanger it seems you remain unconvinced, that pre-1.7 + cljc compatibility is worth maintaining build tooling for. I have no qualms recognizing 1.7 as a minimum targed for "reasonably recent" clojure, based on recent survey numbers (in fact I could rewrite a couple of expressions in data.xml in a much more elegant way). If that's the message we want to send though, why even run test matrices for older versions? EDIT do you dislike including a separate maven plugin, or is it about the approach itself? i.e. does the ticket help?

@puredanger

This comment has been minimized.

Show comment
Hide comment
@puredanger

puredanger Jan 6, 2017

Contributor

@bendlas by default, most of the contribs are not distributed as AOT-compiled jars. However, the default parent pom is configured to AOT compile the project as a check to ensure that it compiles (see https://github.com/clojure/build.poms/blob/master/pom.xml#L112-L114).

The clojure.version is used when performing the main project build on the build box. The test-matrix project will test all Clojure and JDK versions that are specified as supported in the ci_data file (https://github.com/clojure/build.ci/blob/master/ci_data.clj).

I dislike doing source pre-processing. The whole point of having cljc files is to avoid doing source pre-processing like we did with cljx. I also dislike including more plugins as that just adds more infrastructure that needs to be understood, maintained, and updated.

My belief based on the state of the overall ecosystem is that the overwhelming majority of users are using Clojure 1.7+ and thus using cljc files is a sufficient solution. For those that are not, they can continue using older versions of the library.

Contributor

puredanger commented Jan 6, 2017

@bendlas by default, most of the contribs are not distributed as AOT-compiled jars. However, the default parent pom is configured to AOT compile the project as a check to ensure that it compiles (see https://github.com/clojure/build.poms/blob/master/pom.xml#L112-L114).

The clojure.version is used when performing the main project build on the build box. The test-matrix project will test all Clojure and JDK versions that are specified as supported in the ci_data file (https://github.com/clojure/build.ci/blob/master/ci_data.clj).

I dislike doing source pre-processing. The whole point of having cljc files is to avoid doing source pre-processing like we did with cljx. I also dislike including more plugins as that just adds more infrastructure that needs to be understood, maintained, and updated.

My belief based on the state of the overall ecosystem is that the overwhelming majority of users are using Clojure 1.7+ and thus using cljc files is a sufficient solution. For those that are not, they can continue using older versions of the library.

@Engelberg

This comment has been minimized.

Show comment
Hide comment
@Engelberg

Engelberg Jan 6, 2017

Contributor
Contributor

Engelberg commented Jan 6, 2017

@Engelberg

This comment has been minimized.

Show comment
Hide comment
@Engelberg

Engelberg Jan 6, 2017

Contributor
Contributor

Engelberg commented Jan 6, 2017

@Engelberg

This comment has been minimized.

Show comment
Hide comment
@Engelberg

Engelberg Jan 6, 2017

Contributor
Contributor

Engelberg commented Jan 6, 2017

@Engelberg

This comment has been minimized.

Show comment
Hide comment
@Engelberg

Engelberg Jan 6, 2017

Contributor
Contributor

Engelberg commented Jan 6, 2017

@bendlas

This comment has been minimized.

Show comment
Hide comment
@bendlas

bendlas Jan 6, 2017

@puredanger alright, thanks for clarifying that. I'll conform to your stance in data.xml and remove cljc-maven-plugin. TBH, I'd rather invest that time into adding a cljs test runner to clojure-maven-plugin, too.

bendlas commented Jan 6, 2017

@puredanger alright, thanks for clarifying that. I'll conform to your stance in data.xml and remove cljc-maven-plugin. TBH, I'd rather invest that time into adding a cljs test runner to clojure-maven-plugin, too.

@puredanger

This comment has been minimized.

Show comment
Hide comment
@puredanger

puredanger Jan 7, 2017

Contributor

@bendlas I'd also much rather have cljs test runner support :)

Contributor

puredanger commented Jan 7, 2017

@bendlas I'd also much rather have cljs test runner support :)

@Engelberg

This comment has been minimized.

Show comment
Hide comment
@Engelberg

Engelberg Jan 8, 2017

Contributor

Implemented in 0.1.4

Contributor

Engelberg commented Jan 8, 2017

Implemented in 0.1.4

@Engelberg Engelberg closed this Jan 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment