Hello! I created a PR for clr.tools.namespace a couple weeks back, but haven’t gotten anything approvals/feedback on it. Would somebody mind taking a look? Also, in the future, is there a better place to bump stale CLR PRs? https://github.com/clojure/clr.tools.namespace/pull/3
Ah, that makes sense. I'll go through the contributor steps sometime this week (hopefully later today). As far as the JVM implementation, I ran into this issue while running the same code on both JVM and CLR and only the CLR threw the exception. So I don't think the issue exists there, but I'll certainly double check when I get back to this.
I'll try to figure out what the difference is between the two implementations.
Noticed a couple other differences in clr tools last night around repl/scan.
1. ::dir/time is an instant whereas in the JVM implementation it is an an integer.
2. subsequent calls to repl/scan seem to return a different result than the JVM implementation.
Point 1 I'm pretty sure has to do with clr tools. Point 2 I need to dig in a little more and make sure the difference is in fact in clr tools and not just how I'm using it.
Looks like the test passes in CLR, but still failing in JVM. I don’t know enough about the library to make any sure statements about its intent. Though I do think, whatever the intent, the behavior between CLR/JVM should remain the same.
I’ll raise this question in http://ask.clojure.org. Thanks for helping dig into this!
I had forgotten that the PR you posted had a fix for the problem. Even though we're not doing PRs, you do have a CA so I can just edit that in.
Ah, yeah. Kept that PR open in case you still wanted to look at it. I can close it whenever. I did sign the Contributor Agreement. So in the future, it looks like I need to go through the Jira ticketing process before contributing code to master, is that right?
BTW created question in: http://ask.cojure.org https://ask.clojure.org/index.php/14284/clr-behavior-tools-namespace-does-not-match-implementation
Yes, the guidelines at https://clojure.org/dev/dev give the prereqs to be a contributor as having signed the CA (which you've done) and optionally file a contributor support request. The latter gives you the ability to file tickets directly. I assume you need that access also to contribute a patch. Of minor interest: I clicked on the link given on that page for "contributor support request" just to see what they ask. I don't have access to that page so I can't determine what is needed. I'm not sure how one is supposed to file a request.
I can close the PR when I'm done with it.
you are probably being foiled by being logged into jira already - if you try it in a private browser window you should see what someone without access would see. but it just files a request that I get to add a user
I can just short circuit all that by sending Brandon an invite
That did not occur to me. Thx. BTW, I updated CONTRIBUTING.md in all the libraries I'm managing.
My error. I fat-fingered the notice into trash.
I am duty-bound to follow the Clojure protocols regarding submissions to any of the "official" libraries, meaning anything under http://github.com/clojure. As described https://clojure.org/community/contributing#_reporting_problems_and_requesting_enhancements.
They have never accepted pull requests. (I might have violated that once or twice.) They do take patches. Under https://clojure.org/community/contributing#_development, it states: "If you would like to provide a patch on a specific ticket in the jira tracker, please follow the https://clojure.org/dev/dev to become a contributor." I have never taken code from anyone who is not a contributor.
Sorry.
Looking at your PR: the function clojure.tools.namespace/modified-files is a direct translation of the corresponding code in the JVM version of the library. I'm guessing the JVM version would suffer from the same problem. Perhaps something you would like to mention in http://ask.clojure.org?
WRT to test workflow: thanks. Needed across the board. I just haven't had time to deal with it. And automating really is made so much easier now that cljr is available.
Of course, I don't manage the http://github.com/clojure account. I'm not sure how the powers-that-be will feel about me putting CPU cycles on their tab.
Actually, it doesn't appear that any of the clr.* libraries have JIRA projects, so I can't create tickets for those libraries. Perhaps @alexmiller can suggest how to proceed.
trying to hit all the questions above: • contributors - must sign the Contributor Agreement per above • PRs - we do not accept per Rich's preference • issues - Ask + jira is preferred here. I wonder if it makes sense to manage the CLJR issues in the existing CLJ project for the contribs since they are often close, as suggested? We have some contribs that are CLJ + CLJS now and we typically manage them in the same jira project • automation - we are now doing pretty much all our CI and testing automation for Clojure project in Github actions, but still only at a fraction of the free plan minutes in a typical month. so feel free to automate and I will continue keeping an eye on costs and we can have that conversation if we ever need to
• CA -- agreed (as has been my practice) • PRs - agreed (as have been overwhelmingly my practice) • issues - your call. A proliferation of JIRA projects with virtually no traffic doesn't seem useful. As long as having the rare CLR issue in there doesn't bother others, certainly fine with me. As you would typically manage creation of issues, I'm guessing at the same time you could just assign them to me? • Automation -- I'll start slowly and see how it goes. I'll look over what @bwancor has provided and also look at the automation in other CLJ/CLJS libraries. Thanks for the quick response!
on issues, yeah, it's no big deal - usually in mixed projects I will treat CLJ as the default and mark the CLJS (or now CLJR) in the title so they are a bit more visible (the ASYNC project for core.async is the main place this has come up, well some in TDEPS too). let's do that and if it ever becomes onerous I can always make a new project and migrate things
also, fyi, in github we create PR templates that say not to use PRs, not sure if those made it into the cljr repos, but probably a good practice
Those templates did not make it. I'll look into it.
Can you give me a clue of what you are doing/seeing?
Here are a couple tests that pass in JVM and fail in CLR (modeled after the dir-test ).
(ns clojure.tools.namespace.repl-test
(:require [clojure.test :refer [deftest is]]
[clojure.tools.namespace.dir :as dir]
[clojure.tools.namespace.find :as find]
[clojure.tools.namespace.repl :as repl]
[clojure.tools.namespace.test-helpers :as help]))
(deftest t-repl-scan
(let [dir (help/create-temp-dir "t-repl-scan")
_main-clj (help/create-source dir 'example.main :clj '[example.one])
_one-cljc (help/create-source dir 'example.one :clj)
_other-dir (help/create-temp-dir "t-repl-scan-other")
files-1 (::dir/files (repl/scan {:platform find/clj}))
files-2 (::dir/files (repl/scan {:platform find/clj}))]
(is (not-empty files-1))
(is (not-empty files-2))
(is (= files-1 files-2))))
(deftest t-repl-scan-time
(let [scan (repl/scan {:platform find/clj})]
(is (integer? (::dir/time scan)))))
First test shows the CLR missing data in ::dir/files on subsequent calls. I recall seeing missing data elsewhere, but I just haven’t made a test for those yet.
The second test show that the ::dir/time type differs between JVM / CLR. Probably nbd, but maybe an issue since the attribute is exposed to tools.namespace clients.
This does indicate a gap in the coverage of the test suite in tools.namespace (and hence in clr.tools.namespace). Perhaps something to bring up http://ask.clojure.org and contribute?
These tests are failing for me for the same reason, relating to a default value supplied that is not the correct data type. That is related to the fact you point regarding the type difference: java.File.lastModified returns a long (Unix time); System.IO.FileSystem.Info.LastWriteTimeUTC returns a DateTime. Once I fix the default value problem, the next question is whether we use what the platform gives or convert for consistency?
If you want to insert a quick fix to keep playing, replace clojure.tools.namespace.dir/modified-files with:
(defn- modified-files [tracker files]
(filter #(DateTime/op_LessThan ^DateTime (::time tracker System.DateTime/UnixEpoch) (.get_LastWriteTimeUtc ^FileSystemInfo %)) files))I haven't looked at the implementation of namespace in probably a dozen years (and probably didn't really understand all the details of it at that time either), but if I'm reading the source correctly and interpreting the intention of t-repl-scan correctly, I think the following captures that intent:
(deftest t-repl-scan
(try
(let [dir (help/create-temp-dir "t-repl-scan")
_main-clj (help/create-source dir 'example.main :clj '[example.one])
_one-cljc (help/create-source dir 'example.one :clj)
_other-dir (help/create-temp-dir "t-repl-scan-other")
_ (repl/set-refresh-dirs dir _other-dir)
scan1 (repl/scan {:platform find/clj})
scan2 (repl/scan {:platform find/clj})
files-1 (::dir/files scan1)
files-2 (::dir/files scan2)]
(is (= (count files-1) 2))
(is (= (count files-2) 0)))
(finally (repl/clear))))
repl/scan looks for changes in designated directories. Somewhere in the comments in repl, you will find:
The directories to be scanned are controlled by 'set-refresh-dirs';
defaults to all directories on the Java classpath.
So the temp directories created in the test will not be scanned unless you call repl/set-refresh-dirs. I have added that call. Also, because that call has side-effects, it should be undone before leaving the test. So I have wrapped the test in a try and call (repl/clear) in the finally.
Successive scans will not return the same set of files. Only files that have changed.
On the first scan, both example.main.clj and example.one.clj will be found.
On the second scan, because they have not changed, they are not present under ::dir/files
I believe these statements are correct, but defer to anyone more familiar with how this code works. It is not the simplest code to wade through.