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

Make (sift :move) create resources #680

Merged
merged 1 commit into from
Jun 20, 2018

Conversation

bhagany
Copy link
Contributor

@bhagany bhagany commented Jan 8, 2018

I'll be honest - I don't know if this is a good idea, but I think
there is a genuine bug that should be addressed. The problem stems
from calling sync-user-dirs! at the end of the task stack - doing
so clobbers moves files that were moved via (sift :move) back to
their original position on the filesystem, but does not apply the
same transistion to the fileset. Files that have roles do not
suffer this inconsistency.

The result is that any post-wraps in the task stack will find that
files in the fileset they were given don't actually exist on disk.
This is also a problem when serving files from the fileset - I end
up missing any files that were moved with sift.

I'll be honest - I don't know if this is a good idea, but I think
there is a genuine bug that should be addressed. The problem stems
from calling `sync-user-dirs!` at the end of the task stack - doing
so clobbers moves files that were moved via `(sift :move)` back to
their original position on the filesystem, but does not apply the
same transistion to the fileset. Files that have roles do not
suffer this inconsistency.

The result is that any post-wraps in the task stack will find that
files in the fileset they were given don't actually exist on disk.
This is also a problem when serving files from the fileset - I end
up missing any files that were moved with `sift`.
@bhagany
Copy link
Contributor Author

bhagany commented Jan 8, 2018

I should also note that this problem also happens with boot.core/mv, but since I'm not sure what the right fix is, I limited the scope of this PR to await feedback.

@arichiardi
Copy link
Contributor

Side-note, it might be what causes this boot-http problem as well.

@arichiardi
Copy link
Contributor

Also worth noting that if this is the way to go, one alternative untested arguably more readable solution would be to always compose the :move with the :to-resource action.

@bhagany
Copy link
Contributor Author

bhagany commented Jan 8, 2018

Here's a small build.boot I used to reproduce:

(set-env!
 :resource-paths #{"resources"}
 :dependencies '[[boot/core "2.8.0-SNAPSHOT"]])

(require '[boot.core :as boot])

(deftask foo->bar []
  (comp (sift :move {#"foo\.txt" "bar.txt"})
        (fn [next-task]
          (fn [fileset]
            (let [bar (boot/tmp-get fileset "bar.txt")]
              (println (.getPath (boot/tmp-file bar)))
              (Thread/sleep 20000)
              (println "syncing")
              (#'boot/sync-user-dirs!)
              (Thread/sleep 20000)
              (next-task fileset))))))

You'll also need to create a file called resources/foo.txt

I'm manually calling sync-user-dirs! here to show that it's the problem. When run, you'll see that the printed path will exist during the first sleep, and be gone during the second sleep.

@arichiardi
Copy link
Contributor

arichiardi commented Jan 8, 2018

I have tried this branch against my perun project and I have a very weird problem. Basically the classic pattern with watch does not seem to work:

(deftask dev
  [s stage VAL kw "The stage value - :dev, :staging or :prod"]
  (comp (include-js)
        (watch) 
        (blog-less)
        (serve :resource-root "public")
        (notify :audible true)))

It gets stuck at the following without further output (with -v as well):

 boot dev
SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
SLF4J: Defaulting to no-operation (NOP) logger implementation
SLF4J: See http://www.slf4j.org/codes.html#StaticLoggerBinder for further details.
Sifting output files...
Sifting output files...
Sifting output files...

Starting file watcher (CTRL-C to quit)...

Everything works on master.

@bhagany
Copy link
Contributor Author

bhagany commented Jan 8, 2018

@arichiardi do you have the full code somewhere I can see?

@arichiardi
Copy link
Contributor

@bhagany, no but I want to write a smaller sample. Hopefully very soon.

@arichiardi
Copy link
Contributor

arichiardi commented Jan 10, 2018

This still hangs for me with the patch applied. I have not been able to understand why yet.

@arichiardi
Copy link
Contributor

I am going to ridicule myself with the following but 👍 so...your patch works, the fact that I thought it was hanging is a side effect of it: the watch task, instead of immediately triggering and then wait, triggers after the first change to a file happens.

The difference we see in behavior might be due to the platform (I am running on linux).

I think we should strive to preserve the behavior but if we can't I of course prefer to have the fix in.

@alandipert
Copy link
Contributor

Thanks all. This is a solid fix.

@alandipert alandipert merged commit 37fa9fb into boot-clj:master Jun 20, 2018
alandipert added a commit that referenced this pull request Jun 28, 2018
bbatsov pushed a commit to bbatsov/boot that referenced this pull request Jun 29, 2018
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.

4 participants