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

Portable fsmonitor #6718

Closed
FedericoCeratto opened this issue Nov 10, 2017 · 22 comments

Comments

Projects
None yet
7 participants
@FedericoCeratto
Copy link
Member

commented Nov 10, 2017

fsmonitor should support OSX and Windows as well.

@dom96

This comment has been minimized.

Copy link
Member

commented Nov 11, 2017

fsmonitor will be taken out of the stdlib.

@bluenote10

This comment has been minimized.

Copy link
Contributor

commented Nov 11, 2017

Please don't remove it from the core libs. See my comment here why having general solution in the stdlib is an important feature for the build ecosystems.

@dom96

This comment has been minimized.

Copy link
Member

commented Nov 12, 2017

The reason why having it in the standard library could be beneficial: One of the things I'm missing the most compared to Scala development is SBT's ~compile/~run/~test functionality, which allows to easily set-up source code watchers and re-run the compiler (and optionally arbitrary entry points or tests) whenever the source code changes.

Couldn't this be implemented outside of the compiler? Perhaps just as an app that calls the compiler. I don't see much advantage in integrating this with the compiler.

@dom96

This comment has been minimized.

Copy link
Member

commented Nov 12, 2017

Overall though, it's not the end of the world if we remove it. Adding things to the stdlib in the future will be easy once a mature fsmonitor package is created.

@bluenote10

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2017

Couldn't this be implemented outside of the compiler? Perhaps just as an app that calls the compiler. I don't see much advantage in integrating this with the compiler.

Not the compiler but the standard lib :). If it becomes a dependency there will be a chicken/egg problem: A .nimble file is able to define package dependencies, but if the .nimble itself has a dependency (for the file watchers stuff) it is not runnable to resolve the dependencies.

Overall though, it's not the end of the world if we remove it. Adding things to the stdlib in the future will be easy once a mature fsmonitor package is created.

Agreed, since a generic/portable API would probably be much different anyway, it can be re-introduced e.g. under a different name.

@Yardanico Yardanico added the Stdlib label Nov 12, 2017

@dom96

This comment has been minimized.

Copy link
Member

commented Nov 14, 2017

I'm not sure I follow. Are you wanting to integrate it with Nimble?

@bluenote10

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2017

Yes, somehow I want to bring SBT's ~run/~test features (or all these npm watcher approaches) into the Nim world. Makes such a huge difference for test driven development.

@dom96

This comment has been minimized.

Copy link
Member

commented Nov 14, 2017

Okay, so once again: why does this need to be inside nimble? Why can't you build an external app that calls Nimble when appropriate?

@bluenote10

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2017

It's surely possible (see almost all my repos where I have tried various work-around to this), but the development experience lacks compared to built-in support:

  • how to make this external app cross platform?
  • how to handle and install the dependencies of the external app itself?
  • how to handle task forwarding?
  • how to sync knowledge of "triggers" of the external app with the build system itself? In most build systems this knowledge is available only on build system level and is unknown to an external app.

I can only recommend to play around with SBT to see its power. It is just so nice that people can check out any SBT project, run a single, standardized command (sbt ~test) no matter what system they are on, and go into a development mode, where you basically see the immediately effects as you type. I was always trying to offer the same in Nim, but there is currently no easy solution and pushing file monitoring out into a third party dependency would probably make it even harder to accomplish this goal.

@dom96

This comment has been minimized.

Copy link
Member

commented Nov 14, 2017

To be honest I'd be reluctant to add this functionality to Nimble.

how to make this external app cross platform?

Just like any other app? What is it about this task that makes cross platform support more difficult?

how to handle and install the dependencies of the external app itself?

Using Nimble

how to handle task forwarding?

I'm not 100% this is what you mean, but:

execProcess("nimble c blah")

how to sync knowledge of "triggers" of the external app with the build system itself? In most build systems this knowledge is available only on build system level and is unknown to an external app.

Can you give an example of this?


Overall, I am pretty conservative with what makes it into Nimble. Package managers are complex beasts and these additional features only add to the complexity, it'd be better if it was implemented separately.

@bluenote10

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2017

What is it about this task that makes cross platform support more difficult?

Only that a bash solution or even an npm based solution is currently much easier than a Nim solution. Having a nice cross platform solution in the standard library would change that.

What you are suggesting is a custom solution, and won't achieve the single, standardized command property. Every project would have to re-implement the same, and in practice no one will.

I understand your reservation, which is why I'm more looking into possibilities to maybe start out with an addon to Nimble (also the motivation for nim-lang/nimble#427).

@FedericoCeratto

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2017

@bluenote10 is something like this what you are looking for? https://github.com/FedericoCeratto/nim-testrunner/

@bluenote10

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2017

Yes, the test runner goes in the right direction and is pretty cool for running ad-hoc stuff. What I'm really after is the way this is integrated in modern build systems like SBT or Gradle though. The motivation is the same as the Gradle team had in their design:

With Maven, the same watch functionality needs to be implemented for each plugin or you have to use a plugin that has a predefined set of goals. We wanted to do better than that. We wanted any plugin to be able to leverage the power of continuous builds without having to supply additional information. We also wanted the set of tasks to execute to be completely ad-hoc. Since Gradle needs to know a task’s inputs and outputs for incremental builds, we had all the information necessary to start watching for changes.

@FedericoCeratto

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2017

@bluenote10 sounds like you want something that monitor files for changes and gives hints to Nimble and Nim to [re]build in the most efficient way. Perhaps this is the use-case you had in mind for Nimble-as-a-library?

@dom96

This comment has been minimized.

Copy link
Member

commented Apr 27, 2018

I still think we should move fsmonitor out of the stdlib.

@dom96 dom96 added High Priority Easy and removed Feature labels Apr 27, 2018

@zah

This comment has been minimized.

Copy link
Member

commented Apr 28, 2018

Reacting to file system changes should be a feature supported by the async event loop. On Linux for example, the inotify API returns a file descriptor that can be queried for events with the same epoll call sitting at the heart of the event loop.

@ConsoleTVs

This comment has been minimized.

Copy link

commented May 2, 2018

Just wondering why https://nim-lang.org/docs/fsmonitor.html is down...

@dom96

This comment has been minimized.

Copy link
Member

commented May 2, 2018

Because it'll be moved out of the stdlib.

@dom96

This comment has been minimized.

Copy link
Member

commented May 2, 2018

@zah I don't think the event loop needs to know anything about inotify. It's file descriptors all the way down, no?

@zah

This comment has been minimized.

Copy link
Member

commented May 5, 2018

@dom96, if the low-level APIs (such as register(fd)) are enough to implement fsmonitor on top of asyncdispatch, then yes, but IMO we need to investigate this before we banish fsmonitor in user-land.

@dom96

This comment has been minimized.

Copy link
Member

commented May 5, 2018

We can always add fsmonitor back to the stdlib. As it is currently it shouldn't be used anyway as it relies on a deprecated async implementation.

@dom96 dom96 added the Hacktoberfest label Oct 2, 2018

dom96 added a commit that referenced this issue Oct 27, 2018

@dom96

This comment has been minimized.

Copy link
Member

commented Oct 27, 2018

Please move to https://github.com/nim-lang/needed-libraries/issues if it's not already there. This will be closed once #9529 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.