-
Notifications
You must be signed in to change notification settings - Fork 256
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
Adding Lockfile #728
base: main
Are you sure you want to change the base?
Adding Lockfile #728
Conversation
Known limitations with the current implementation is as follows,
|
This seems like an interesting addition, but since we've linked this in a discussion with #727, I'll note that it is not an ergonomic solution for the problem that PR solves. They might be complementary, though! OT seeks the ability to dynamically swap implementations of virtual cores, so a set of targets expressed by in-tree fusesoc cores may have parts of the dependency tree swapped out for out-of-tree users. With the lockfile alone, we end up needing tooling to generate this file on the fly, something that is considerably more complex than simply adding required VLNVs on the command line (especially since this file covers much more than the virtual core mapping). |
I think lock files and providing the VLNV per command line serve two different purposes. From the OT perspective we are interested in solving the following scenario. An IP depends on a virtual core, i.e., the primitive library. We then wanto to to have an easy way (without core file change) to build this IP with different core providers. This can be for example the public |
Of course! I realize I didn't think through the use-cases properly. I do agree that it's complementary though. I would even suspect there are compelling use-cases for allowing a virtual/concrete mapping in the target sections for some use-cases as well. With that, I think the implementation in #727 makes sense but I'm thinking of some tweaks for consistency. Will write in more detail there. For this PR it would still be good to know if the lock file functionality matches what you are looking for. |
With the related discussion in #727 I suggest we do this in two steps. Let's focus this PR on reading and applying an existing (manually written) lockfile. For this, we also need the ability to specify the name of the lockfile to load. Once that is in place, we can figure out the remaining use cases for writing and updating lock files. |
I have changed the implementation to take a file path as lockfile argument, |
Could FuseSoC look for a lockfile in the 'cores root' directory unless overridden with a |
If no lockfile argument is supplied, FuseSoC could go through cores root directories and look for a specific file, |
Probably better to have a set place for it like most other package managers (e.g. Cargo, npm, uv, nix flake). They all have a top level config file (e.g. Cargo.toml) and in the same directory a lock file (e.g. Cargo.lock). In FuseSoC's case, the top-level config is Also for this to properly lock down dependencies we should really have hashes (normally sha256) for each core. |
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.
My first pass through the code. I haven't actually run the code.
I don't think we need the virtuals section. I think we only need a list of cores. A valid lock file will have an implementation for each virtual core dependency in it's list of cores.
I also don't think we should allow for partial lock files. I don't know of any package manager that allows them, and I don't really see the point of only partially locking down dependencies.
This looks closer to a requirements/dependencies format (e.g. requirements.txt) than a lockfile. Most language package managers seem to agree on a way to do this, touched upon in #728 (comment). They express requirements in their config (e.g. pyproject.toml
) then lock this down with an exact version of each library/core accompanied by a hash, source location and dependencies in a lock file (e.g. uv.lock
). I think this way of managing dependencies would work well for FuseSoC.
fusesoc/lockfile.py
Outdated
if "cores" in lockfile_data: | ||
cores = [Vlnv(core_name) for core_name in lockfile_data["cores"]] | ||
if "virtuals" in lockfile_data: | ||
for virtual_name, core_name in lockfile_data["virtuals"].items(): | ||
virtuals[Vlnv(virtual_name)] = Vlnv(core_name) |
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.
Nitpick: I would write this as
cores = [Vlnv(core_name) for core_name in lockfile_data.setdefault("cores", [])]
virtuals = {
Vlnv(virtual_name) : Vlnv(core_name)
for virtual_name, core_name in lockfile_data.setdefault("virtuals", {}).items()
}
Also I would exit early if the lockfile path isn't a file, so lockfile_data
is never None.
if not lockfile_path.is_file():
raise ValueError(f"{lockfile_path} is not a file.")
@HU90m All valid concerns. I'll add my comments to each of the topics. hashes: We want hashes for several reasons. Signing of cores is the main thing I'm thinking about which is becoming more relevant once we have a larger body of publicly available cores, but also detect modified caches etc. But we don't have support for that yet, and I think we should aim to have a first version of lock files without that and extend with hashes and source file locations later. partial lock-files: I personally see great value in this in situations where you need a specific version of a core instead of the one that FuseSoC would pick by default, e.g. to temporary work around tool bugs. It's good that you bring up requirements.txt, because I believe my vision is probably somewhere between that and cargo/poetry lock files. Lock file location: The FuseSoC use model is quite different from the sw package managers in this regard. virtuals in lock files: You said we don't need the virtuals section at all because the cores section will tell us which concrete implementation to use. I think that could work under the following assumptions.
One problem however arises when you have two concrete implementations implementing the same virtual because you can't explicitly tell which one to use. And I do think that's a valid use-case. That was the idea of the two sections, |
tests/test_coremanager.py
Outdated
@@ -349,7 +349,6 @@ def test_virtual_non_deterministic_virtual(caplog): | |||
core_manager=cm, | |||
work_root=work_root, | |||
) | |||
edalizer.run() |
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.
Is this an intentional change, and if so, is it relevant to the other code or should it be in a separate commit?
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.
It is intentional, but not relevant to the other changes. I will leave it out.
- Removed lockfile_store - Removed invalidation of lock file
Okie, happy for hashes to be added in a later.
Can you not just specify the specific version of the core you want to depend on in the core file. or are you wanting to override a core file's choice?
I don't think the lock file should be for manually setting mappings. Or if it is it shouldn't be called a lock file to avoid confusion with other tools lock files which are only ever meant to be generated. |
Current FuseSoC OntologyAdded this section so you can check my understanding of FuseSoC at the moment.
Specifying core versions and virtual core mappings
Adding a lock filePersonally, I would like a lock file to function on the library level to manage external dependencies like in other package managers. It could be associated with a Prior ArtCargo.toml# This file is automatically @generated by Cargo.
# It is not intended for manual editing.
version = 3
[[package]]
name = "aho-corasick"
version = "1.1.3"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "8e60d3430d3a69478ad0993f19238d2df97c507009a52b3c10addcd7f6bcb916"
dependencies = [
"memchr",
]
[[package]]
name = "cc"
version = "1.1.13"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "72db2f7947ecee9b03b510377e8bb9077afa27176fdbff55c51027e976fdcc48"
dependencies = [
"shlex",
] uv.lockversion = 1
requires-python = ">=3.6, <4"
[[package]]
name = "attrs"
version = "22.2.0"
source = { registry = "https://pypi.org/simple" }
sdist = { url = "https://files.pythonhosted.org/packages/21/31/3f468da74c7de4fcf9b25591e682856389b3400b4b62f201e65f15ea3e07/attrs-22.2.0.tar.gz", hash = "sha256:c9227bfc2f01993c03f68db37d1d15c9690188323c067c641f1a35ca58185f99", size = 215900 }
wheels = [
{ url = "https://files.pythonhosted.org/packages/fb/6e/6f83bf616d2becdf333a1640f1d463fef3150e2e926b7010cb0f81c95e88/attrs-22.2.0-py3-none-any.whl", hash = "sha256:29e95c7f6778868dbd49170f98f8818f78f3dc5e0e37c0b1f474e3561b240836", size = 60018 },
]
[[package]]
name = "click"
version = "8.0.4"
source = { registry = "https://pypi.org/simple" }
dependencies = [
{ name = "colorama", marker = "platform_system == 'Windows'" },
{ name = "importlib-metadata", marker = "python_full_version < '3.8'" },
]
sdist = { url = "https://files.pythonhosted.org/packages/dd/cf/706c1ad49ab26abed0b77a2f867984c1341ed7387b8030a6aa914e2942a0/click-8.0.4.tar.gz", hash = "sha256:8458d7b1287c5fb128c90e23381cf99dcde74beaf6c7ff6384ce84d6fe090adb", size = 329520 }
wheels = [
{ url = "https://files.pythonhosted.org/packages/4a/a8/0b2ced25639fb20cc1c9784de90a8c25f9504a7f18cd8b5397bd61696d7d/click-8.0.4-py3-none-any.whl", hash = "sha256:6a7a62563bbfabfda3a38f3023a1db4a35978c0abd76f6c9605ecd6554d6d9b1", size = 97486 },
] package-lock.json{
"name": "gubbins-compiler",
"version": "0.0.0",
"lockfileVersion": 3,
"requires": true,
"packages": {
"": {
"name": "gubbins-compiler",
"version": "0.0.0",
"dependencies": {
"@djot/djot": "^0.3.2",
"happy-dom": "^16.5.3",
"highlight.js": "^11.11.1",
"mime": "^4.0.6",
"temml": "^0.10.32",
"ws": "^8.18.0"
}
},
"node_modules/@djot/djot": {
"version": "0.3.2",
"resolved": "https://registry.npmjs.org/@djot/djot/-/djot-0.3.2.tgz",
"integrity": "sha512-joMKR24B8rxueyFiJbpZAqEiypjvOyzTxzkhyr0q5mM/sUBaOD3unna/9IxtOotFugViyYlkIRaiXg3xM//zxg==",
"license": "MIT",
"bin": {
"djot": "lib/cli.js"
},
"engines": {
"node": ">=17.0.0"
}
},
"node_modules/happy-dom": {
"version": "16.6.0",
"resolved": "https://registry.npmjs.org/happy-dom/-/happy-dom-16.6.0.tgz",
"integrity": "sha512-Zz5S9sog8a3p8XYZbO+eI1QMOAvCNnIoyrH8A8MLX+X2mJrzADTy+kdETmc4q+uD9AGAvQYGn96qBAn2RAciKw==",
"license": "MIT",
"dependencies": {
"webidl-conversions": "^7.0.0",
"whatwg-mimetype": "^3.0.0"
},
"engines": {
"node": ">=18.0.0"
}
}
}
} A fusesoc-y lock formatThe FuseSoC lock file could look something like this (after all the features are added):
version: 1
libraries:
- name: "fuseoc-cores"
location: "."
url: "git+https://github.com/fusesoc/fusesoc-cores"
rev: "cb967637dbf7cee25117203bbdf9c10b62dfb25a"
hash: "sha256-0JLnk+qr991aKWjzTrG4JE0L5e2NrcOstT8+ggjzKr0="
cores:
- "::serv:1.0.0"
- "::serv:1.0.2"
- name: "opentitan-ip"
location: "hw/ip/"
url: "git+ssh://github.com/lowRISC/opentitan"
rev: "f7ce10b62dfb2e2511cb967637db7203bbdf9c5a"
hash: "sha256-1a0L5e2NrcOsKWjzTrG4JEtT8+ggjzKr00JLnk+qr99="
cores:
- "lowrisc:ip:aes:1.0"
- name: "opentitan-prim-generic"
location: "hw/prim/generic"
url: "git+ssh://github.com/lowRISC/opentitan"
rev: "f7ce10b62dfb2e2511cb967637db7203bbdf9c5a"
hash: "sha256-1a0L5e2NrcOsKWjzTrG4JEtT8+ggjzKr00JLnk+qr99="
cores:
- "lowrisc:prim:sram"
providers:
- core: "::serv:1.0.0"
url: "git+ssh://github.com/olofk/serve"
rev: "b2e2511cb967637db7203bbdf9c5af7ce10b62df"
hash: "sha256-KWjzTrG4J0JLnk+qr9EtT8+ggjzKr091a0L5e2NrcOs="
# Not required if the dependency resolution is always consistent,
# but may be nice to include.
cores:
- name: "lowrisc:ip:aes:1.0"
location: "hw/ip/aes/aes.core"
dependencies:
- "lowrisc:ip:tlul:0.1"
- "lowrisc:ip:lc_ctrl_pkg:0.1"
- "lowrisc:ip:edn_pkg:0.1"
- "lowrisc:ip:keymgr_pkg:0.1"
- name: "lowrisc:ip:tlul:0.1"
location: "hw/ip/tlul/tlul.core"
dependencies:
- "lowrisc:tlul:socket_1n:0.1"
- "lowrisc:tlul:socket_m1:0.1"
- "lowrisc:tlul:adapter_sram:0.1" Associated with a [library.fusesoc-cores]
sync-uri = https://github.com/fusesoc/fusesoc-cores
sync-type = git
rev = "cb967637dbf7"
[library.opentitan-ip]
sync-uri = https://github.com/lowRISC/opentitan
sync-type = git
location = "hw/ip/"
rev = "Earlgrey-PROD.M6"
[library.opentitan-prim-generic]
sync-uri = https://github.com/lowRISC/opentitan
sync-type = git
location = "hw/prim/generic"
rev = "Earlgrey-PROD.M6"
[library.a-local-library]
location = "rtl/" A TOML version (not proposing, but adding for those more toml literate).version = 1
[[library]]
order = 0
name = "fusesoc-cores"
location = "."
url = "git+https://github.com/fusesoc/fusesoc-cores"
rev = "cb967637dbf7cee25117203bbdf9c10b62dfb25a"
hash = "sha256-0JLnk+qr991aKWjzTrG4JE0L5e2NrcOstT8+ggjzKr0="
cores = [
"::serv:1.0.0",
"::serv:1.0.2",
# ...
]
[[library]]
order = 1
name = "opentitan-ip"
location = "hw/ip/"
url = "git+ssh://github.com/lowRISC/opentitan"
rev = "f7ce10b62dfb2e2511cb967637db7203bbdf9c5a"
hash = "sha256-1a0L5e2NrcOsKWjzTrG4JEtT8+ggjzKr00JLnk+qr99="
cores = [
"lowrisc:ip:aes:1.0",
# ...
]
[[library]]
order = 2
name = "opentitan-prim-generic"
location = "hw/prim/generic"
url = "git+ssh://github.com/lowRISC/opentitan"
rev = "f7ce10b62dfb2e2511cb967637db7203bbdf9c5a"
hash = "sha256-1a0L5e2NrcOsKWjzTrG4JEtT8+ggjzKr00JLnk+qr99="
cores = [
# ...
]
[[provider]]
core = "::serv:1.0.0"
url = "git+ssh://github.com/olofk/serve"
rev = "b2e2511cb967637db7203bbdf9c5af7ce10b62df"
hash = "sha256-KWjzTrG4J0JLnk+qr9EtT8+ggjzKr091a0L5e2NrcOs="
# Not required if the dependency resolution is always consistent,
# but may be nice to include.
[[core]]
name = "lowrisc:ip:aes:1.0"
location = "hw/ip/aes/aes.core"
dependencies = [
"lowrisc:ip:tlul:0.1",
"lowrisc:ip:lc_ctrl_pkg:0.1",
"lowrisc:ip:edn_pkg:0.1",
"lowrisc:ip:keymgr_pkg:0.1",
# ...
]
[[core]]
name = "lowrisc:ip:tlul:0.1"
location = "hw/ip/tlul/tlul.core"
dependencies = [
"lowrisc:tlul:socket_1n:0.1",
"lowrisc:tlul:socket_m1:0.1",
"lowrisc:tlul:adapter_sram:0.1",
# ...
] |
FuseSoC Lockfile
Format
The format contains a list of locked down versions for the cores in the design. There is also a map describing mapping of virtual cores in the design.
Schema
Following is the JSON schema for the lock file format.
File location
The file is located at the current working directory of the
fusesoc
run.Command line argument
An command line argument has been added to
fusesoc run
,lockfile
. Thelockfile
argument can hawe following values wereenable
is the default.--lockfile="enable"
--lockfile="disable"
--lockfile="reset"
Enable
Enables the use of lock file. If there is no lock file to be found one will be generated during the
fusesoc
run. If there is a lock file, the file will be read and used to influence the core selection during the run.Core version selection
If there are dependencies which relation is not locked down to a single version, the use of lock file will change that relation to the one in the lock file. If the range of versions described by the dependecy is outside that described in the lock file, the lock file version will not be used and the lock file will be marked as invalid.
Virtual core selection
The virual core mapping in the lock file will be applied to virtual core dependecies during core selection.
Invalidation
During the fusesoc run, the validity of the lock file is evaluated. If invalid versions, missing cores are detected the lock file is invalidated. When invalidated the lock file is re-written at the end of the run.
Disable
The use of lock file will be disabled, no loading or storing of lock file.
Reset
No lock file is loaded. A new lock file is stored after core selection.
Use cases
Locking down a core dependency
The user might want to lock down a dependency, by writing a simple lock file with the core that shall be locked down.
Shared development
Multiple developers developing a design, might want to share lock file to limit any dependecy ambiguities.