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.

Sign up for GitHub

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

Jump to bottom

The module lockfile doesn't contain hashes of files used in extension impl functions #19068

Closed
Wyverald opened this issue Jul 25, 2023 · 12 comments
Closed

The module lockfile doesn't contain hashes of files used in extension impl functions #19068

Wyverald opened this issue Jul 25, 2023 · 12 comments
Assignees
@SalmaSamy
Labels
area-Bzlmod Bzlmod-specific PRs, issues, and feature requests P1 I'll work on this now. (Assignee required) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug
Milestone
7.0.0 branch cut

Comments

@Wyverald
Copy link
Member 

def _my_ext_impl(mctx):
  do_something(mctx.read(Label("//:abc.txt")))
my_ext = module_extension(_my_ext_impl)

If //:abc.txt changes, my_ext should be re-run.
@Wyverald Wyverald added type: feature request P1 I'll work on this now. (Assignee required) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. area-Bzlmod Bzlmod-specific PRs, issues, and feature requests labels Jul 25, 2023
@Wyverald Wyverald added this to the 7.0.0 branch cut milestone Jul 25, 2023
@Wyverald Wyverald added this to Bzlmod Jul 25, 2023
@github-project-automation github-project-automation bot moved this to Todo in Bzlmod Jul 25, 2023
@Wyverald
Copy link
Member Author

@bazel-io fork 6.4.0

@bazel-io bazel-io mentioned this issue Jul 25, 2023
Closed
@brentleyjones
Copy link
Contributor

Since the lockfile is enabled by default now in 6.3.0, is this high enough priority to fix in 6.3.1? How big of an impact is this bug (doesn't seem like a feature request)?

@Wyverald
Copy link
Member Author

Hmm. I wonder if people consider this bad enough to warrant disabling lockfiles by default again. My gut feeling is that it's too big of a fix to go in 6.3.1, but am not 100% on that. @meteorcloudy WDYT?

@fmeum
Copy link
Collaborator

fmeum commented Jul 25, 2023

Does it rerun when a file referenced by label from a tag changes? If not, then go_deps.from_file(go_mod = "//:go.mod") would no longer see any changes to Go deps, which would be bad. I can test this tomorrow.

@Wyverald
Copy link
Member Author

It wouldn't. Essentially any file read by the extension impl function wouldn't be noticed (unless the Bazel server process is alive, in which case Skyframe handles it).

I'm more and more leaning towards marking this a bug instead ... in fact I'll just go ahead and do it.

@meteorcloudy
Copy link
Member

Yes, I'm inclined to also backport the fix to 6.3 as well. Trying to use the lockfile feature for Bazel itself with 6.3.0, this bug makes it quite awkward.

@Wyverald
Copy link
Member Author

@bazel-io fork 6.3.0

@Wyverald
Copy link
Member Author

@bazel-io fork 6.3.1

@bazel-io bazel-io mentioned this issue Jul 26, 2023
Closed
@Wyverald Wyverald mentioned this issue Jul 27, 2023
Merged
@Wyverald Wyverald removed this from the 6.3.1 release blockers milestone Jul 27, 2023
@fmeum fmeum mentioned this issue Jul 27, 2023
Closed
@github-project-automation github-project-automation bot moved this from Todo to Done in Bzlmod Aug 1, 2023
@iancha1992
Copy link
Member

iancha1992 commented Aug 1, 2023 •
edited
Loading

@Wyverald @SalmaSamy
When cherry-picking this to release-6.4.0, There are some merge conflicts with some files.

  1. src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelLockFileValue.java
    Conflicts with the getModuleExtensionDiff function

  2. src/main/java/com/google/devtools/build/lib/bazel/bzlmod/LockFileModuleExtension.java
    public abstract ImmutableMap<String, String> getEnvVariables() should already have been part of the LockFileModuleExtension class in the release branch.
    We also have conflicts with the create function

  3. src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java
    Conflicts with the singleExtensionEvalValue variable

We may need another commit/PR to address these before we can cherry-pick dd3ae79
cc: @bazelbuild/triage

@Wyverald
Copy link
Member Author

Wyverald commented Aug 1, 2023

thanks, Ian. @SalmaSamy could you send a cherry-pick PR later?

@SalmaSamy
Copy link
Contributor

Sure!

SalmaSamy added a commit that referenced this issue Aug 3, 2023
@SalmaSamy
Re-run extension if its files change
0ba107b 
fixes #19068

PiperOrigin-RevId: 552823534
Change-Id: I87256b2cf954b932e24c70e22386020599f21a6f
@iancha1992 iancha1992 mentioned this issue Aug 3, 2023
Merged
iancha1992 added a commit that referenced this issue Aug 4, 2023
@SalmaSamy @iancha1992
[6.4.0] Lockfile updates & fixes (#19153)
1bc0f87 
* Add 'environ' attribute to module extensions and update lockfile

- Add 'environ' to module extensions
- Store the variables and their values in the lockfile
- Compare the variables and values with the lockfile and re-evaluate or error if they differ

PiperOrigin-RevId: 548964193
Change-Id: Ic2d52fe3332e93095c414d8bca4c9b4312bca8c2

# Conflicts:
#	src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtension.java
#	src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java
#	src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java
#	src/main/java/com/google/devtools/build/lib/skyframe/ActionEnvironmentFunction.java
#	src/main/java/com/google/devtools/build/lib/skyframe/ClientEnvironmentFunction.java
#	src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/RepositoryModuleApi.java
#	src/main/java/com/google/devtools/build/skydoc/fakebuildapi/repository/FakeRepositoryModule.java
#	src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java
#	src/test/py/bazel/bzlmod/bazel_lockfile_test.py

* Remove extra changes

* Re-run extension if its files change
fixes #19068

PiperOrigin-RevId: 552823534
Change-Id: I87256b2cf954b932e24c70e22386020599f21a6f

* Do not fail on new fields added to lockfile data
Fixes #19105

PiperOrigin-RevId: 553068023
Change-Id: I877bc8ece0641c01119a9295e09175a2d0a3a0c1

---------

Co-authored-by: Ian (Hee) Cha <[email protected]>
iancha1992 added a commit to iancha1992/bazel that referenced this issue Aug 4, 2023
@SalmaSamy @iancha1992
[6.4.0] Lockfile updates & fixes (bazelbuild#19153)
5e9d3e3 
* Add 'environ' attribute to module extensions and update lockfile

- Add 'environ' to module extensions
- Store the variables and their values in the lockfile
- Compare the variables and values with the lockfile and re-evaluate or error if they differ

PiperOrigin-RevId: 548964193
Change-Id: Ic2d52fe3332e93095c414d8bca4c9b4312bca8c2

# Conflicts:
#	src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtension.java
#	src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java
#	src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java
#	src/main/java/com/google/devtools/build/lib/skyframe/ActionEnvironmentFunction.java
#	src/main/java/com/google/devtools/build/lib/skyframe/ClientEnvironmentFunction.java
#	src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/RepositoryModuleApi.java
#	src/main/java/com/google/devtools/build/skydoc/fakebuildapi/repository/FakeRepositoryModule.java
#	src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java
#	src/test/py/bazel/bzlmod/bazel_lockfile_test.py

* Remove extra changes

* Re-run extension if its files change
fixes bazelbuild#19068

PiperOrigin-RevId: 552823534
Change-Id: I87256b2cf954b932e24c70e22386020599f21a6f

* Do not fail on new fields added to lockfile data
Fixes bazelbuild#19105

PiperOrigin-RevId: 553068023
Change-Id: I877bc8ece0641c01119a9295e09175a2d0a3a0c1

---------

Co-authored-by: Ian (Hee) Cha <[email protected]>
@iancha1992 iancha1992 mentioned this issue Aug 4, 2023
Merged
@iancha1992
Copy link
Member

@bazel-io fork 6.3.2

@bazel-io bazel-io mentioned this issue Aug 4, 2023
Closed
iancha1992 added a commit that referenced this issue Aug 4, 2023
@iancha1992 @SalmaSamy
[6.4.0] Lockfile updates & fixes (#19153) (#19175)
9cd182e 
* Add 'environ' attribute to module extensions and update lockfile

- Add 'environ' to module extensions
- Store the variables and their values in the lockfile
- Compare the variables and values with the lockfile and re-evaluate or error if they differ

PiperOrigin-RevId: 548964193
Change-Id: Ic2d52fe3332e93095c414d8bca4c9b4312bca8c2

# Conflicts:
#	src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtension.java
#	src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java
#	src/main/java/com/google/devtools/build/lib/bazel/repository/starlark/StarlarkRepositoryModule.java
#	src/main/java/com/google/devtools/build/lib/skyframe/ActionEnvironmentFunction.java
#	src/main/java/com/google/devtools/build/lib/skyframe/ClientEnvironmentFunction.java
#	src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/RepositoryModuleApi.java
#	src/main/java/com/google/devtools/build/skydoc/fakebuildapi/repository/FakeRepositoryModule.java
#	src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java
#	src/test/py/bazel/bzlmod/bazel_lockfile_test.py

* Remove extra changes

* Re-run extension if its files change
fixes #19068

PiperOrigin-RevId: 552823534
Change-Id: I87256b2cf954b932e24c70e22386020599f21a6f

* Do not fail on new fields added to lockfile data
Fixes #19105

PiperOrigin-RevId: 553068023
Change-Id: I877bc8ece0641c01119a9295e09175a2d0a3a0c1

---------

Co-authored-by: Salma Samy <[email protected]>
@cgrindel cgrindel mentioned this issue Aug 10, 2023
Merged
cgrindel added a commit to cgrindel/rules_swift_package_manager that referenced this issue Aug 16, 2023
@cgrindel
chore: disable Bazel lockfiles and remove them (#508)
2c8b471 
Related to bazelbuild/bazel#19068.
@Wyverald Wyverald added this to the 7.0.0 branch cut milestone Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@SalmaSamy SalmaSamy

Labels
area-Bzlmod Bzlmod-specific PRs, issues, and feature requests P1 I'll work on this now. (Assignee required) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug
Projects
Bzlmod
Archived in project
Milestone
7.0.0 branch cut
Development

No branches or pull requests
6 participants
@brentleyjones @Wyverald @meteorcloudy @fmeum @SalmaSamy @iancha1992