-
Notifications
You must be signed in to change notification settings - Fork 205
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
Drop LF < 1.14 from supported damlc output versions #11701
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
# Copyright (c) 2021 Digital Asset (Switzerland) GmbH and/or its affiliates. All rights reserved. | ||
# SPDX-License-Identifier: Apache-2.0 | ||
|
||
load("@os_info//:os_info.bzl", "is_windows", "os_name") | ||
load("@io_bazel_rules_scala//scala:scala_cross_version.bzl", "default_maven_server_urls") | ||
|
||
runfiles_library = """ | ||
# Copy-pasted from the Bazel Bash runfiles library v2. | ||
set -uo pipefail; f=bazel_tools/tools/bash/runfiles/runfiles.bash | ||
source "${RUNFILES_DIR:-/dev/null}/$f" 2>/dev/null || \ | ||
source "$(grep -sm1 "^$f " "${RUNFILES_MANIFEST_FILE:-/dev/null}" | cut -f2- -d' ')" 2>/dev/null || \ | ||
source "$0.runfiles/$f" 2>/dev/null || \ | ||
source "$(grep -sm1 "^$f " "$0.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \ | ||
source "$(grep -sm1 "^$f " "$0.exe.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \ | ||
{ echo>&2 "ERROR: cannot find $f"; exit 1; }; f=; set -e | ||
# --- end runfiles.bash initialization v2 --- | ||
""" | ||
|
||
def _damlc_legacy_impl(ctx): | ||
out_dir = ctx.path("damlc") | ||
|
||
ctx.download_and_extract( | ||
output = out_dir, | ||
url = "https://github.com/digital-asset/daml/releases/download/v{}/daml-sdk-{}-{}.tar.gz".format(ctx.attr.version, ctx.attr.version, ctx.attr.os_name), | ||
sha256 = ctx.attr.sha256[ctx.attr.os_name], | ||
stripPrefix = "sdk-{}/damlc".format(ctx.attr.version), | ||
) | ||
|
||
ctx.file( | ||
"damlc.sh", | ||
content = | ||
"""#!/usr/bin/env bash | ||
{runfiles_library} | ||
$(rlocation damlc_legacy/damlc/damlc.exe) $@ | ||
""".format(runfiles_library = runfiles_library), | ||
) if is_windows else None | ||
|
||
ctx.file( | ||
"BUILD", | ||
content = | ||
""" | ||
load("@os_info//:os_info.bzl", "is_windows") | ||
package(default_visibility = ["//visibility:public"]) | ||
sh_binary( | ||
name = "damlc_legacy", | ||
srcs = [":damlc/damlc"], | ||
) if not is_windows else sh_binary( | ||
name = "damlc_legacy", | ||
srcs = [":damlc.sh"], | ||
deps = ["@bazel_tools//tools/bash/runfiles"], | ||
data = ["damlc/damlc.exe"], | ||
) | ||
""".format(version = ctx.attr.version), | ||
) | ||
return None | ||
|
||
damlc_legacy = repository_rule( | ||
implementation = _damlc_legacy_impl, | ||
attrs = { | ||
"version": attr.string(mandatory = True), | ||
"os_name": attr.string(mandatory = False, default = os_name), | ||
"sha256": attr.string_dict(mandatory = True), | ||
}, | ||
) |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -59,9 +59,12 @@ PROTO_LF_VERSIONS = LF_VERSIONS | |||||
# The subset of LF versions accepted by //daml-lf/encoder | ||||||
ENCODER_LF_VERSIONS = ["1.dev" if ver == "dev" else ver for ver in LF_VERSIONS] | ||||||
|
||||||
# We support older LF versions using an older compiler binary | ||||||
LEGACY_COMPILER_LF_VERSIONS = ENCODER_LF_VERSIONS | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This also contains Maybe we should hardcode the list of versions supported by the legacy compiler, since it presumably isn't going to keep up with new LF versions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking off this more as legacy + newer versions, i.e., everything we can build in |
||||||
|
||||||
# The subset of LF versions accepted by the compiler in the syntax | ||||||
# expected by the --target option. | ||||||
COMPILER_LF_VERSIONS = ENCODER_LF_VERSIONS | ||||||
COMPILER_LF_VERSIONS = [ver for ver in ENCODER_LF_VERSIONS if ver not in ["1.6", "1.7", "1.8", "1.11", "1.12", "1.13"]] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not using the comparator defined at line
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed the code already There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The list If so you could rewrite this as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed it to that and added There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! |
||||||
|
||||||
# We need Any in DAML Script so we require DAML-LF >= 1.7 | ||||||
SCRIPT_LF_VERSIONS = [ver for ver in COMPILER_LF_VERSIONS if ver != "1.6"] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this definition anymore ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch, killed it b356687 |
||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not dropping this line as done in the previous file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missed that 5a3ba1e