Skip to content

Commit

Permalink
chore: deprecate built_pkgs_with_manifest() (FuelLabs#4447)
Browse files Browse the repository at this point in the history
closes FuelLabs#4374

@kayagokalp rightfully mentioned in the issue that the package
descriptor is already contained within the `BuiltPackage` struct, which
means we do not need this representation anymore:

> This is a nice abstractions but it made me think a little and I think
returning Vec of (PackageManifestFile, Arc<BuiltPackage>) might be
unnecessary at the first place from built_pkg_with_manifest function.
> 
> Every built package contains a package descriptor which contains the
related package manifest so returning a tuple is kind of a data
duplication.

## Description


## Checklist

- [x] I have linked to any relevant issues.
- [ ] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [ ] I have added tests that prove my fix is effective or that my
feature works.
- [ ] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [ ] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.
  • Loading branch information
spiral-ladder authored Apr 17, 2023
1 parent 7b4aee5 commit 911c8b3
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 54 deletions.
26 changes: 13 additions & 13 deletions forc-plugins/forc-client/src/op/deploy.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::{
cmd,
util::{
pkg::built_pkgs_with_manifest,
pkg::built_pkgs,
tx::{TransactionBuilderExt, TX_SUBMIT_TIMEOUT_MS},
},
};
Expand Down Expand Up @@ -85,17 +85,15 @@ pub async fn deploy(command: cmd::Deploy) -> Result<Vec<DeployedContract>> {
};

let build_opts = build_opts_from_cmd(&command);
let built_pkgs_with_manifest = built_pkgs_with_manifest(&curr_dir, build_opts)?;
let built_pkgs = built_pkgs(&curr_dir, build_opts)?;

let contract_salt_map = if let Some(salt_input) = &command.salt {
// If we're building 1 package, we just parse the salt as a string, ie. 0x00...
// If we're building >1 package, we must parse the salt as a pair of strings, ie. contract_name:0x00...
if built_pkgs_with_manifest.len() > 1 {
if built_pkgs.len() > 1 {
let map = validate_and_parse_salts(
salt_input.clone(),
built_pkgs_with_manifest
.iter()
.map(|bpwm| bpwm.package_manifest_file()),
built_pkgs.iter().map(|b| &b.descriptor.manifest_file),
)?;

Some(map)
Expand All @@ -113,8 +111,9 @@ pub async fn deploy(command: cmd::Deploy) -> Result<Vec<DeployedContract>> {
.unwrap();
let mut contract_salt_map = ContractSaltMap::default();
contract_salt_map.insert(
built_pkgs_with_manifest[0]
.package_manifest_file()
built_pkgs[0]
.descriptor
.manifest_file
.project_name()
.to_string(),
salt,
Expand All @@ -125,15 +124,16 @@ pub async fn deploy(command: cmd::Deploy) -> Result<Vec<DeployedContract>> {
None
};

for built in built_pkgs_with_manifest {
let member_manifest = built.package_manifest_file();
if member_manifest
for pkg in built_pkgs {
if pkg
.descriptor
.manifest_file
.check_program_type(vec![TreeType::Contract])
.is_ok()
{
let salt = match (&contract_salt_map, command.random_salt) {
(Some(map), false) => {
if let Some(salt) = map.get(member_manifest.project_name()) {
if let Some(salt) = map.get(pkg.descriptor.manifest_file.project_name()) {
*salt
} else {
Default::default()
Expand All @@ -146,7 +146,7 @@ pub async fn deploy(command: cmd::Deploy) -> Result<Vec<DeployedContract>> {
}
};
let contract_id =
deploy_pkg(&command, member_manifest, built.built_package(), salt).await?;
deploy_pkg(&command, &pkg.descriptor.manifest_file, &pkg, salt).await?;
contract_ids.push(contract_id);
}
}
Expand Down
11 changes: 6 additions & 5 deletions forc-plugins/forc-client/src/op/run.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::{
cmd,
util::{
pkg::built_pkgs_with_manifest,
pkg::built_pkgs,
tx::{TransactionBuilderExt, TX_SUBMIT_TIMEOUT_MS},
},
};
Expand Down Expand Up @@ -37,14 +37,15 @@ pub async fn run(command: cmd::Run) -> Result<Vec<RanScript>> {
std::env::current_dir().map_err(|e| anyhow!("{:?}", e))?
};
let build_opts = build_opts_from_cmd(&command);
let built_pkgs_with_manifest = built_pkgs_with_manifest(&curr_dir, build_opts)?;
let built_pkgs_with_manifest = built_pkgs(&curr_dir, build_opts)?;
for built in built_pkgs_with_manifest {
let member_manifest = built.package_manifest_file();
if member_manifest
if built
.descriptor
.manifest_file
.check_program_type(vec![TreeType::Script])
.is_ok()
{
let pkg_receipts = run_pkg(&command, member_manifest, built.built_package()).await?;
let pkg_receipts = run_pkg(&command, &built.descriptor.manifest_file, &built).await?;
receipts.push(pkg_receipts);
}
}
Expand Down
46 changes: 10 additions & 36 deletions forc-plugins/forc-client/src/util/pkg.rs
Original file line number Diff line number Diff line change
@@ -1,59 +1,33 @@
use std::{collections::HashMap, path::Path, sync::Arc};

use anyhow::{bail, Result};
use anyhow::Result;
use forc_pkg::{self as pkg, manifest::ManifestFile, BuildOpts, BuildPlan};
use pkg::{build_with_options, BuiltPackage, PackageManifestFile};
use pkg::{build_with_options, BuiltPackage};

#[derive(Clone, Debug)]
pub struct BuiltPackageWithManifest(Arc<BuiltPackage>, PackageManifestFile);

impl BuiltPackageWithManifest {
/// Returns an immutable reference into the Arc<BuiltPackage>.
pub fn built_package(&self) -> &Arc<BuiltPackage> {
&self.0
}

/// Returns an immutable reference into the PackageManifestFile.
pub fn package_manifest_file(&self) -> &PackageManifestFile {
&self.1
}
}

pub(crate) fn built_pkgs_with_manifest(
path: &Path,
build_opts: BuildOpts,
) -> Result<Vec<BuiltPackageWithManifest>> {
pub(crate) fn built_pkgs(path: &Path, build_opts: BuildOpts) -> Result<Vec<Arc<BuiltPackage>>> {
let manifest_file = ManifestFile::from_dir(path)?;
let mut member_manifests = manifest_file.member_manifests()?;
let lock_path = manifest_file.lock_path()?;
let build_plan = BuildPlan::from_lock_and_manifests(
&lock_path,
&member_manifests,
&manifest_file.member_manifests()?,
build_opts.pkg.locked,
build_opts.pkg.offline,
)?;
let graph = build_plan.graph();
let built = build_with_options(build_opts)?;
let mut built_pkgs: HashMap<&pkg::Pinned, Arc<_>> = built.into_members().collect();
let mut pkgs_with_manifest = Vec::new();
let mut members: HashMap<&pkg::Pinned, Arc<_>> = built.into_members().collect();
let mut built_pkgs = Vec::new();

for member_index in build_plan.member_nodes() {
let pkg = &graph[member_index];
let pkg_name = &pkg.name;
// Check if the current member is built.
//
// For individual members of the workspace, member nodes would be iterating
// over all the members but only the relevant member would be built.
if let Some(built_pkg) = built_pkgs.remove(pkg) {
let member_manifest = member_manifests
.remove(pkg_name)
.expect("Member manifest file is missing");
pkgs_with_manifest.push(BuiltPackageWithManifest(built_pkg, member_manifest));
if let Some(built_pkg) = members.remove(pkg) {
built_pkgs.push(built_pkg);
}
}

if pkgs_with_manifest.is_empty() {
bail!("No built packages collected");
}

Ok(pkgs_with_manifest)
Ok(built_pkgs)
}

0 comments on commit 911c8b3

Please sign in to comment.