bazel_7: 7.4.0 -> 7.4.1 #355723
bazel_7: 7.4.0 -> 7.4.1 #355723
Conversation
|
@ofborg build bazel_7 |
|
@ofborg build bazel_7 |
|
build fails at patchPhase. based on nixos-unstable-small 18979df |
boltzmannrain
force-pushed
the
bazel_7.4.1
branch
from
affb62c to
82c4c76
Compare
|
Looks like build with So that's indeed a regression but small-scale. Patch needs to be updated, originally the conflicting part about markers seems to be coming from #186106 Not yet sure what's it should do and how to do it on latest bazel codebase, will try to have a look |
|
And looks like the marker hack is required for |
|
But if we also remove removal of .marker files from buildBazelPackage deps outputs it seems to be building fcitx5-mozc |
ofborg
bot
added
10.rebuild-darwin: 1-10
10.rebuild-darwin: 1
10.rebuild-linux: 1-10
labels
|
This is preventing me from upgrading my systems to the NixOS 24.11 pre-release. I'll review this and backport it. |
|
Result of 2 packages failed to build:
1 package built:
|
|
This PR itself doesn't fix fcitx5-mozc build (buildBazelBackage with bazel=bazel-7), I don't yet fully understand the patch used there to update it. Found a little more background on the markers hack here #65374 It seems that removing a section of failing patch + some more changes may work for fcitx5-mozc as in boltzmannrain@14d715d#diff-fd4e3dd34e0c6b6d8da7ac1dc25530696d40844760fd3aebf103e2444c55c6a8R163 But I don't know if that change to buildBazelPackage setup works for other packages, maybe for bazel 5/6 we should keep existing behavior of removing markers to be on the safe side (building tensorflow might be a good test) Additionally if we decide to change buildBazelPackage behavior it probably makes sense to invalidate fixed output derivations, similarly how they get invalidated on bazel version changes in |
Yeah, it looks like the problem is the patch from |
There was a problem hiding this comment.
Can you try this patch:
nix-hacks-7.patch
diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java
index 53e6494..09052cd 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryDelegatorFunction.java
@@ -55,6 +55,7 @@ import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import java.io.IOException;
+import java.util.Collections;
import java.util.Map;
import java.util.Optional;
import java.util.TreeMap;
@@ -193,18 +194,8 @@ public final class RepositoryDelegatorFunction implements SkyFunction {
}
if (shouldUseCachedRepos(env, handler, repoRoot, rule)) {
- // Make sure marker file is up-to-date; correctly describes the current repository state
- byte[] markerHash = digestWriter.areRepositoryAndMarkerFileConsistent(handler, env);
- if (env.valuesMissing()) {
- return null;
- }
- if (markerHash != null) { // repo exist & up-to-date
- return RepositoryDirectoryValue.builder()
- .setPath(repoRoot)
- .setDigest(markerHash)
- .setExcludeFromVendoring(excludeRepoFromVendoring)
- .build();
- }
+ // Nix hack: Always consider cached dirs as up-to-date
+ return RepositoryDirectoryValue.builder().setPath(repoRoot).setDigest(digestWriter.writeMarkerFile(Collections.emptyMap())).build();
}
/* At this point: This is a force fetch, a local repository, OR The repository cache is old or
@@ -634,11 +625,12 @@ public final class RepositoryDelegatorFunction implements SkyFunction {
builder.append(escape(key)).append(" ").append(escape(value)).append("\n");
}
String content = builder.toString();
- try {
- FileSystemUtils.writeContent(markerPath, UTF_8, content);
- } catch (IOException e) {
- throw new RepositoryFunctionException(e, Transience.TRANSIENT);
- }
+ // Nix hack: Do not write these pesky marker files
+ //try {
+ // FileSystemUtils.writeContent(markerPath, UTF_8, content);
+ //} catch (IOException e) {
+ // throw new RepositoryFunctionException(e, Transience.TRANSIENT);
+ //}
return new Fingerprint().addString(content).digestAndReset();
}
diff --git a/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java b/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java
index 649647c..64d05b5 100644
--- a/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/shell/JavaSubprocessFactory.java
@@ -165,7 +165,6 @@ public class JavaSubprocessFactory implements SubprocessFactory {
}
builder.command(argv);
if (params.getEnv() != null) {
- builder.environment().clear();
builder.environment().putAll(params.getEnv());
}You can conditionally import it just for 7.x.x like this:
# See enableNixHacks argument above.
++ lib.optional enableNixHacks (
if versionAtLeast version "7" then ./nix-hacks-7.patch else ./nix-hacks.patch
);I'm not really sure if the Collections.emptyMap() hack here have any serious implications, but fcitx5 does build now.
The problem was that we need to get the recorded inputs via readMarkerFile and markerPath now, but they're both private. Maybe we could patch areRepositoryAndMarkerFileConsistent to make sure it always succeeds, but I'm not sure if it'd have any unwanted side effects.
|
Please don't use substring to check versions, we have |
|
Do we still need the patch? fcit5-mozc is the only package explicitly depending on bazel_7, and the build succeeds with this commit that removes the patch and .marker file removal. boltzmannrain@14d715d We could try migrating other packages to bazel_7 to see if it works, but they might break on bazel_7 regardless. |
I'm not really sure either, but I've had experiences where bazel builds ok once, but fails in subsequent builds (due to hash changes). The PR you linked seems to imply that. Can you try clearing your cache and rebuilding it to see if it works?
Yes for now. fcitx5-mozc doesn't actually need bazel 7 right now, but it will be a hard requirement for both fcitx5-mozc and mozc on the next version bump (because it uses |
`bazel_7` upgrade to 7.3.1+ had a regression breaking `buildBazelPackage { bazel = bazel_7; }`.
See some discussion in NixOS#355723
`fcitx5-mozc` package should be currently buildable with bazel 6
NixOS#355723 (comment)
Hopefully by the time it requires 7+ `buildBazelPackage` will be fixed
So let's downgrade bazel for now to 6.x for this package to unblock the 24.11 release
Should fix NixOS#355852
It feels safer though to use For bazel_7&buildBazelPackage see following items to be improved
|
| + { | ||
| + // Nix hack: Always consider cached dirs as up-to-date | ||
| return RepositoryDirectoryValue.builder() | ||
| .setPath(repoRoot) | ||
| - .setDigest(markerHash) | ||
| - .setExcludeFromVendoring(shouldExcludeRepoFromVendoring(handler, rule)) | ||
| - .build(); | ||
| - } | ||
| + // Nix hack: Always consider cached dirs as up-to-date | ||
| + return RepositoryDirectoryValue.builder().setPath(repoRoot).setDigest(digestWriter.writeMarkerFile()).build(); | ||
| } | ||
|
|
||
| /* At this point: This is a force fetch, a local repository, OR The repository cache is old or | ||
| @@ -610,11 +600,12 @@ public final class RepositoryDelegatorFunction implements SkyFunction { | ||
| builder.append(escape(key)).append(" ").append(escape(value)).append("\n"); | ||
| - .setExcludeFromVendoring(excludeRepoFromVendoring) | ||
| + .setDigest(digestWriter.writeMarkerFile(Collections.emptyMap())) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
Have you tested if the patch works? It seems to generate an extra scope:
if (shouldUseCachedRepos(env, handler, repoRoot, rule)) {
{
// Nix hack: Always consider cached dirs as up-to-date
return RepositoryDirectoryValue.builder()
.setPath(repoRoot)
.setDigest(digestWriter.writeMarkerFile(Collections.emptyMap()))
.build();
}
}Does this actually return from the function or just from the scope (not really familiar with Java)?
There was a problem hiding this comment.
Extra nested block creates a nested lexical scope but return still refers to the enclosing function as there's no block-level returns. Added it to reduce formatting indentation diff against bazel upstream
This comment was marked as outdated.
This comment was marked as outdated.
|
|
Successfully created backport PR for |
`bazel_7` upgrade to 7.3.1+ had a regression breaking `buildBazelPackage { bazel = bazel_7; }`.
See some discussion in NixOS#355723
`fcitx5-mozc` package should be currently buildable with bazel 6
NixOS#355723 (comment)
Hopefully by the time it requires 7+ `buildBazelPackage` will be fixed
So let's downgrade bazel for now to 6.x for this package to unblock the 24.11 release
Should fix NixOS#355852
`bazel_7` upgrade to 7.3.1+ had a regression breaking `buildBazelPackage { bazel = bazel_7; }`.
See some discussion in #355723
`fcitx5-mozc` package should be currently buildable with bazel 6
#355723 (comment)
Hopefully by the time it requires 7+ `buildBazelPackage` will be fixed
So let's downgrade bazel for now to 6.x for this package to unblock the 24.11 release
Should fix #355852
(cherry picked from commit 186054d)
`bazel_7` upgrade to 7.3.1+ had a regression breaking `buildBazelPackage { bazel = bazel_7; }`.
See some discussion in NixOS#355723
`fcitx5-mozc` package should be currently buildable with bazel 6
NixOS#355723 (comment)
Hopefully by the time it requires 7+ `buildBazelPackage` will be fixed
So let's downgrade bazel for now to 6.x for this package to unblock the 24.11 release
Should fix NixOS#355852
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.