Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

Artifact ids #882

Merged
merged 12 commits into from
Feb 18, 2022
Merged

Artifact ids #882

merged 12 commits into from
Feb 18, 2022

Conversation

lattejed
Copy link
Contributor

@lattejed lattejed commented Feb 8, 2022

@mattsse this is related to foundry-rs/foundry#662

This just adds in an ArtifactId and has into_artifacts return it.

I think this warrants ongoing discussion, but I've continued that in the PR linked above.

(There are some unrelated warnings and errors here that I've left alone)

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is great,

before we can merge this:

  • needs a companion on foundry as this breaks, so we can merge both at once
  • some derives

@@ -15,6 +15,36 @@ use std::{
path::{Path, PathBuf},
};

/// Represents unique artifact metadata for identifying artifacts on output
#[derive(Debug, Clone)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also needs PartialEq Eq Hash so we can use them as keys

@lattejed
Copy link
Contributor Author

lattejed commented Feb 9, 2022

Thanks @mattsse, good idea to make them key-able.

Please look at the discussion here it's related to this. Trying to clarify a couple of things.

@lattejed
Copy link
Contributor Author

@mattsse I'm adding a multi-version compile test to this for adding versioned linking. I've found an issue in Graph::resolve_multiple_versions where needed versions are getting pruned incorrectly, which results in a failed compile. Do you happen to know what that might be?

@lattejed
Copy link
Contributor Author

Worth pointing out that I'm not sure this is a regression since the same branch works ok with foundry locally. Possibly a config issue? I'm pretty sure I've dealt with those already tho.

@mattsse
Copy link
Collaborator

mattsse commented Feb 18, 2022

Good catch, you probably found an existing issue.
with this repro I can debug this.

But I think we can go ahead with it anyway? so let's ignore the can_get_versioned_linkrefs until I found the issue.

what's missing to complete foundry-rs/foundry#662 ?

@gakonst gakonst marked this pull request as ready for review February 18, 2022 14:46
Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. issue seems unrelated, i ignored the failing test, fixed fmt'ing and fixed the remaining failing tests. let's get this over the line so we merge the foundry PR, and we can follow up with

  1. Making Project accept filters
  2. Fix the linking stuff

@gakonst gakonst merged commit d8e5e53 into gakonst:master Feb 18, 2022
gakonst added a commit to lattejed/foundry that referenced this pull request Feb 18, 2022
gakonst added a commit to lattejed/foundry that referenced this pull request Feb 18, 2022
gakonst added a commit to lattejed/foundry that referenced this pull request Feb 18, 2022
gakonst added a commit to foundry-rs/foundry that referenced this pull request Feb 18, 2022
* Added test_path method to TestFilter

* Added path regex to test interface

* Added source path filtering to MultiContractRunner

* Updated test Filter and reorganiezed test_helpers

* Updated tests to use new filter

* Fixed test filter

* Use new into_artifacts

* Path filtering requires absolute path

* Formatting

* Fixed warnings

* Minor refactoring

* Minor refactoring

* Bumped semver to 1.0.5 for dev compatibility with ethers-rs

* Added passing test for foundry_utils::link

* Renamed test

* chore: bump ethers for latest artifacts update
gakonst/ethers-rs#882

driveby fixes:
gakonst/ethers-rs#930
gakonst/ethers-rs#928

Co-authored-by: Georgios Konstantopoulos <[email protected]>
charisma98 added a commit to charisma98/foundry that referenced this pull request Mar 4, 2023
* Added test_path method to TestFilter

* Added path regex to test interface

* Added source path filtering to MultiContractRunner

* Updated test Filter and reorganiezed test_helpers

* Updated tests to use new filter

* Fixed test filter

* Use new into_artifacts

* Path filtering requires absolute path

* Formatting

* Fixed warnings

* Minor refactoring

* Minor refactoring

* Bumped semver to 1.0.5 for dev compatibility with ethers-rs

* Added passing test for foundry_utils::link

* Renamed test

* chore: bump ethers for latest artifacts update
gakonst/ethers-rs#882

driveby fixes:
gakonst/ethers-rs#930
gakonst/ethers-rs#928

Co-authored-by: Georgios Konstantopoulos <[email protected]>
0129general added a commit to 0129general/FoundryProject that referenced this pull request May 8, 2024
* Added test_path method to TestFilter

* Added path regex to test interface

* Added source path filtering to MultiContractRunner

* Updated test Filter and reorganiezed test_helpers

* Updated tests to use new filter

* Fixed test filter

* Use new into_artifacts

* Path filtering requires absolute path

* Formatting

* Fixed warnings

* Minor refactoring

* Minor refactoring

* Bumped semver to 1.0.5 for dev compatibility with ethers-rs

* Added passing test for foundry_utils::link

* Renamed test

* chore: bump ethers for latest artifacts update
gakonst/ethers-rs#882

driveby fixes:
gakonst/ethers-rs#930
gakonst/ethers-rs#928

Co-authored-by: Georgios Konstantopoulos <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants