Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

Buckd does not detect build file changes when symbolic links are involved #341

Closed
davido opened this issue Jun 12, 2015 · 16 comments
Closed
Labels

Comments

@davido
Copy link
Contributor

davido commented Jun 12, 2015

Gerrit Code Review project uses Buck and can be extended with plugin concept. There are two plugin Buck build modes supported: in Gerrit tree and standalone (Bucklets driven).

I'm seeing weird stale artifact problem in tree plugin build mode: the plugin is only built once. After that, changing the sources doesn't trigger re-build of the artifact anymore. buck clean doesn't help either, only rm -rf buck-out.

I have a very small reproducer, but will try to describe the problem:

Main project directory structure is

gerrit
|--bucklets/gerrit_plugin.bucklet
|--plugins

Plugin:

foo
|--BUCK
|--src/main/[...]

The content of bucklets/gerrit_plugin.bucklet is:

def gerrit_library(
    name,
    srcs = []):
  java_library(
    name = name,
    srcs = srcs,
    visibility = ['PUBLIC'],
  )

The content of foo/BUCK is:

include_defs('//bucklets/gerrit_plugin.bucklet')

gerrit_library(
  name = 'foo',
  srcs = glob(['src/main/java/**/*.java']),
)

Now, when the plugin foo is linked to plugins directory with symbolic link, we can only build once and never again:

$ cd gerrit
$ buck build plugins/foo:foo
Using buckd.
[-] PROCESSING BUCK FILES...FINISHED 0,1s
[-] BUILDING...FINISHED 0,2s (1/1 JOBS, 1 UPDATED, 0,0% CACHE HITS)

Now when we change the source file: plugins/foo/src/main/java/org/ostrovsky/buck/Main.java and say introduce some compile errors, and re-run, nothing happens:

$ buck build plugins/foo:foo
Using buckd.
[-] PROCESSING BUCK FILES...FINISHED 0,1s
[-] BUILDING...FINISHED 0,0s (1/1 JOBS, 0 UPDATED)

buck clean doesn't help either. The only way to re-trigger the build is to drop buck-out directory:

$ rm -rf buck-out/
$ buck build plugins/foo:foo
Using buckd.
[-] PROCESSING BUCK FILES...FINISHED 0,1s
[+] BUILDING...0,0s (0/1 JOBS, 0 UPDATED)
 |=> //plugins/foo:foo...  0,0s (running javac[0,0s])
/home/davido/projects/buck_problem/plugins/foo/src/main/java/org/ostrovsky/buck/Main.java:21: error: illegal start of type
  42
  ^
/home/davido/projects/buck_problem/plugins/foo/src/main/java/org/ostrovsky/buck/Main.java:21: error: ';' expected
  42
    ^
Errors: 2. Warnings: 0.
BUILD FAILED: //plugins/foo:foo failed with exit code 1:

This problem only occurs, when foo is a symbolic link to some directory parallel to gerrit:

$ ls -all plugins/
lrwxrwxrwx 1 davido users    9 Jun 12 22:26 foo -> ../../foo

All is fine when foo is a real directory and not a symbolic link.

@davido
Copy link
Contributor Author

davido commented Jun 13, 2015

cc @wez

So I had outdated watchman version installed on this machine: 2.9.8. Current is 3.2.0. Unfortunately, watchman upgrade havn't fixed this bug. However after disabling watchman all was working as expected:

$ buck build plugins/foo:foo
Not using buckd because watchman isn't installed.
[-] PROCESSING BUCK FILES...FINISHED 0,0s
[-] BUILDING...FINISHED 0,2s (1/1 JOBS)

Introduce compile error and re-run:

$ buck build plugins/foo:foo
Not using buckd because watchman isn't installed.
[-] PROCESSING BUCK FILES...FINISHED 0,0s
[+] BUILDING...0,3s (0/1 JOBS)
 |=> //plugins/foo:foo...  0,1s (running javac[0,1s])
/home/davido/projects/buck_problem/plugins/foo/src/main/java/org/ostrovsky/buck/Main.java:21: error: illegal start of type
  42
  ^

@davido davido changed the title Stale artifact problem when building plugins linked with symbolic link to main project Watchman erroneously thinks that nothing was changed when symbolic links are involved Jun 13, 2015
@wez
Copy link

wez commented Jun 13, 2015

Watchman does not follow symlinks. facebook/watchman#105 has some context. We have no plans to magically and generally make symlinks "work" because it seems way too complex to make it work as you might expect. We may be able to work something out that will make it easier for tools to deal with this situation though.

In the absence of any helpers, it sounds like buck should resolve symlink(s) and watch-project on the target path(s) and feed those subscriptions into its watcher implementation.

We'll need to brainstorm on this.

@Coneko
Copy link

Coneko commented Jun 13, 2015

We have logic in the parser to not use the in memory cache when symlinks are involved, but it only applies to input files, not to build files themselves.

https://github.com/facebook/buck/blob/master/src/com/facebook/buck/parser/Parser.java#L1138-L1150

This is something we should fix in Buck.

@sdwilsh sdwilsh added the bug label Jun 16, 2015
uwolfer pushed a commit to gerrit-review/gerrit that referenced this issue Jun 17, 2015
Buck has currently one critical bug when Buck daemon is used in
combination with symbolic links: [1]. Document the workaround not
to use Buck daemon in this scenario.

[1] facebook/buck#341

Change-Id: I7cdc19c7245b499e71196d16aab0785dda187fa3
@Coneko Coneko changed the title Watchman erroneously thinks that nothing was changed when symbolic links are involved Buckd does not detect build file changes when symbolic links are involved Jun 18, 2015
@zivkov
Copy link

zivkov commented Oct 20, 2015

I also hit this issue a couple of times when building Gerrit with plugins. Sometimes I want to just symlink a plugin from Gerrit's plugin folder instead of having to clone it inside the plugins folder. Unfortunately, this bug prevents me from doing that.

@somehibs
Copy link

I suppose the suggested workaround at present is to uninstall watchman when working on symblinked dependencies.

@Coneko
Copy link

Coneko commented Feb 29, 2016

You can set NO_BUCKD=1 as an environment variable to stop Buck from using watchman, then you won't need to uninstall it.

@davido
Copy link
Contributor Author

davido commented Feb 29, 2016

Isn't the variable name is NO_BUCKD?

@Coneko
Copy link

Coneko commented Feb 29, 2016

Yes, NO_BUCKD is correct, I edited the comment.

@davido
Copy link
Contributor Author

davido commented Aug 23, 2016

Was this issue fixed? After upgrading to 6a19cf9 we are getting this warning:

Disabling caching for target //plugins/wip:wip__plugin, because one or
more input files are under a symbolic link
({plugins/wip=/home/davido/projects/wip}). This will severely impact
performance! To resolve this, use separate rules and declare
dependencies instead of using symbolic links.

@Coneko
Copy link

Coneko commented Aug 23, 2016

It should be fixed in the sense that it will never be cached now, so you should be able to use buckd, but there's no way of disabling that warning if you want to keep that behaviour.

There is a setting to whitelist certain paths to be symlinks and be cached, but that's for symlinking into read only file systems.

@davido
Copy link
Contributor Author

davido commented Aug 24, 2016

Thanks for fixing it.

[...] but there's no way of disabling that warning if you want to keep that behaviour.

Given that this is a normal workflow, at least in our use cases of Gerrit Code Review plugins, I would expect existence of some i_know_what_i_am_doing setting that silence those warnings.

@Coneko
Copy link

Coneko commented Aug 24, 2016

Oh sorry, my bad, there actually is.

It's project.allow_symlinks: https://buckbuild.com/concept/buckconfig.html#project.allow_symlinks .

@davido
Copy link
Contributor Author

davido commented Aug 25, 2016

Unfortunately it doesn't silent the warning. If I'm reading the documentation correctly, the default for project.allow_symlinks is allow, but:

$ buck --version
buck version 6a19cf921c107ea0f1751e4f84c639e5050e2da7
$ watchman --version
4.3.0

And wip plugin is a symbolic link:

$ ls -all plugins/wip
lrwxrwxrwx 1 davido users 9 Aug 22 08:42 plugins/wip -> ../../wip

and i do not have the mentioned option in .buckconfig:

$ grep allow_symlinks .buckconfig

Still the warning is not suppressed during the build:

$ buck build plugins/wip
Waiting for Watchman query [[watch-project, /home/davido/projects/gerrit/.]]...
Disabling caching for target //plugins/wip:wip__plugin, because one or more input files are under a symbolic link ({plugins/wip=/home/davido/projects/wip}). This will severely impact performance! To resolve this, use separate rules and declare dependencies instead of using symbolic links.
[-] PROCESSING BUCK FILES...FINISHED 0.5s [100%] 🐳  Symlink caused cache invalidation
[-] DOWNLOADING... (0.00 B/S AVG, TOTAL: 0.00 B, 0 Artifacts)
[-] BUILDING...FINISHED 18.9s [100%] (259/259 JOBS, 258 UPDATED, 67 [25.9%] CACHE MISS)

To reproduce, clone Gerrit Code Review from here: [1] and Work In Progress plugin from here: [2], and link wip directory in gerrit's plugins directory.

[1] https://gerrit.googlesource.com/gerrit
[2] https://gerrit.googlesource.com/plugins/wip

@Coneko
Copy link

Coneko commented Aug 25, 2016

The default isn't allow, it's warn. Let me fix the docs.

@davido
Copy link
Contributor Author

davido commented Aug 25, 2016

Thanks.

@davido davido closed this as completed Aug 25, 2016
lucamilanesio pushed a commit to GerritCodeReview/gerrit that referenced this issue Sep 21, 2016
This version fixed a major issue: [1] that was a reason of frustration
of many plugin developers: Not cache sources files under symbolic link.
Now for all such source files, the warning is issued:

"
Disabling caching for target //plugins/wip:wip__plugin, because one or
more input files are under a symbolic link
({plugins/wip=/home/davido/projects/wip}). This will severely impact
performance! To resolve this, use separate rules and declare
dependencies instead of using symbolic links.
"

To suppress this warning we add project.allow_symlink option. This
doesn't have any impact for gerrit core but silences the warning above
when plugins are built in gerrit tree mode.

As pointed out in this issue: [2], we are using some artifacts as source
to the java_library() rule as well as binary_jar for prebuilt_ja rule.
To avoid the warning, we rename sources to have "-sources.jar" suffix
and we rename *.zip to end with .jar in other places.

"
Assuming edit.src.zip is a JAR and renaming to edit.src.jar in
//gerrit-patch-jgit:edit_src. Change the extension of the binary_jar to
'.jar' to remove this warning.
"

source_under_test attribute was removed from java_test() rule.
Replication and cookbook-plugin are updated as well.

local.properties support was removed, but we use it only for download
process customization in our own python script, so that we can keep it
usage and not need to move it to .buckconfig.local.

[1] facebook/buck#341
[2] facebook/buck#855

Change-Id: Idf76cc71c21df43e808179b645f9175767b322a8
@jaimeagudo
Copy link
Contributor

Docs arent' fixed yet guys #1189 Anyway it would be nice to solve this

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

7 participants