Skip to content
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

Fix #42: use patches for revisions #51

Closed

Conversation

yvan-sraka
Copy link
Contributor

No description provided.

app/Foliage/CmdBuild.hs Outdated Show resolved Hide resolved
app/Foliage/CmdBuild.hs Outdated Show resolved Hide resolved
app/Foliage/CmdBuild.hs Outdated Show resolved Hide resolved
app/Foliage/CmdBuild.hs Outdated Show resolved Hide resolved
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

looks plausible now!

app/Foliage/CmdBuild.hs Outdated Show resolved Hide resolved
app/Foliage/CmdBuild.hs Outdated Show resolved Hide resolved
app/Foliage/CmdBuild.hs Outdated Show resolved Hide resolved
app/Foliage/CmdBuild.hs Outdated Show resolved Hide resolved
michaelpj
michaelpj previously approved these changes Apr 26, 2023
andreabedini
andreabedini previously approved these changes Apr 26, 2023
Copy link
Member

@andreabedini andreabedini left a comment

Choose a reason for hiding this comment

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

Looks good 👍

app/Foliage/CmdBuild.hs Outdated Show resolved Hide resolved
@yvan-sraka yvan-sraka self-assigned this May 3, 2023
@yvan-sraka yvan-sraka linked an issue May 3, 2023 that may be closed by this pull request
@andreabedini andreabedini dismissed stale reviews from michaelpj and themself May 3, 2023 08:44

Doesn't seem to work

@andreabedini
Copy link
Member

andreabedini commented May 3, 2023

I tested this PR against CHaP and it looks like there are some issues.

❯ diff -r _repo _repo_old/
Binary files _repo/01-index.tar and _repo_old/01-index.tar differ
Binary files _repo/01-index.tar.gz and _repo_old/01-index.tar.gz differ
...
❯ diff -u <(tar tv --utc --full-time -f _repo/01-index.tar)  <(tar tv --utc --full-time -f _repo_old/01-index.tar) 
--- /dev/fd/63	2023-05-03 16:45:40.965278579 +0800
+++ /dev/fd/62	2023-05-03 16:45:40.965278579 +0800
@@ -586,7 +586,7 @@
 -rw-r--r-- foliage/foliage   243 2023-02-21 23:26:04 cardano-crypto-praos/2.1.1.0/package.json
 -rw-r--r-- foliage/foliage   242 2023-02-21 23:26:04 cardano-crypto-tests/2.1.0.0/package.json
 -rw-r--r-- foliage/foliage   247 2023-02-21 23:26:04 cardano-strict-containers/0.1.2.0/package.json
--rw-r--r-- foliage/foliage 21816 2023-02-22 19:09:40 plutus-core/1.1.1.0/plutus-core.cabal
+-rw-r--r-- foliage/foliage 21826 2023-02-22 19:09:40 plutus-core/1.1.1.0/plutus-core.cabal
 -rw-r--r-- foliage/foliage 20226 2023-02-23 15:00:31 ouroboros-consensus/0.2.1.0/ouroboros-consensus.cabal
 -rw-r--r-- foliage/foliage   242 2023-02-23 15:00:31 ouroboros-consensus/0.2.1.0/package.json
 -rw-r--r-- foliage/foliage  3430 2023-02-23 15:00:35 ouroboros-consensus-mock/0.2.1.0/ouroboros-consensus-mock.cabal
@@ -701,56 +701,56 @@
 -rw-r--r-- foliage/foliage   234 2023-03-10 07:05:55 freer-extras/1.2.0.0/package.json
 -rw-r--r-- foliage/foliage   235 2023-03-10 07:05:55 plutus-ledger/1.2.0.0/package.json
 -rw-r--r-- foliage/foliage   241 2023-03-10 07:05:55 plutus-script-utils/1.2.0.0/package.json
--rw-r--r-- foliage/foliage  2728 2023-03-13 06:49:06 freer-extras/1.2.0.0/freer-extras.cabal
--rw-r--r-- foliage/foliage  5537 2023-03-13 07:04:28 plutus-ledger/1.2.0.0/plutus-ledger.cabal
--rw-r--r-- foliage/foliage  3743 2023-03-13 07:15:21 plutus-script-utils/1.2.0.0/plutus-script-utils.cabal
--rw-r--r-- foliage/foliage  3650 2023-03-15 16:30:48 cardano-crypto-class/2.0.0.1/cardano-crypto-class.cabal
+-rw-r--r-- foliage/foliage  3007 2023-03-13 06:49:06 freer-extras/1.2.0.0/freer-extras.cabal
+-rw-r--r-- foliage/foliage  6363 2023-03-13 07:04:28 plutus-ledger/1.2.0.0/plutus-ledger.cabal
+-rw-r--r-- foliage/foliage  3876 2023-03-13 07:15:21 plutus-script-utils/1.2.0.0/plutus-script-utils.cabal
+-rw-r--r-- foliage/foliage  3600 2023-03-15 16:30:48 cardano-crypto-class/2.0.0.1/cardano-crypto-class.cabal
 -rw-r--r-- foliage/foliage  2290 2023-03-21 09:28:25 cardano-ping/0.1.0.0/cardano-ping.cabal
...

@yvan-sraka can you have a look?

@yvan-sraka yvan-sraka force-pushed the patches-revisions branch 2 times, most recently from fd5a51d to 9ecb7da Compare May 5, 2023 15:07
@yvan-sraka
Copy link
Contributor Author

I tested this PR against CHaP and it looks like there are some issues.

I encountered another set of issues:

yvan@X230 ~/GitHub/foliage (git)-[patches-revisions] % nix run ".#" -- build --current-time 2023-05-05T07:16:57Zwarning: Git tree '/home/yvan/GitHub/foliage' is dirty
warning: Using saved setting for 'extra-substituters = https://cache.iog.io https://foliage.cachix.org https://cache.zw3rk.com' from ~/.local/share/nix/trusted-settings.json.
warning: Using saved setting for 'extra-trusted-public-keys = hydra.iohk.io:f/Ea+s+dFdN+3Y/G+FDgSq+a5NEWhJGzdjvKNGv0/EQ= foliage.cachix.org-1:kAFyYLnk8JcRURWReWZCatM9v3Rk24F5wNMpEj14Q/g= loony-tools:pr9m4BkM/5/eSTZlkQyRt57Jz7OMBxNSUiMC4FkcNfk=' from ~/.local/share/nix/trusted-settings.json.
🌿 Foliage
# buildAction
Current time set to 2023-05-05T07:16:57Z.
# curl (for RemoteAsset https://github.com/input-output-hk/ouroboros-network/tarball/fa10cb4eef1e7d3e095cec3c2bb1210774b7e5fa)
# _sources/typed-protocols/0.1.0.0/revisions/2.cabal
foliage: Error when running Shake build system:
  at want, called at app/Foliage/CmdBuild.hs:45:7 in main:Foliage.CmdBuild
* Depends on: buildAction
  at need, called at app/Foliage/Shake.hs:38:3 in main:Foliage.Shake
* Depends on: _sources/typed-protocols/0.1.0.0/revisions/2.cabal
  at error, called at src/Development/Shake/Internal/Rules/File.hs:179:58 in shake-0.19.7-GFPtSLQxwCcJDXDlNacG7C:Development.Shake.Internal.Rules.File
* Raised the exception:
Error, file does not exist and no rule available:
  _sources/typed-protocols/0.1.0.0/revisions/2.cabal

https://github.com/andreabedini/foliage/pull/51/files#diff-1d7875671c3268865954e891560603f56d5f27866a54862952968dda690026efR85-R107

I'm struggling to determine the best strategy to fix this, knowing that using temporary files is unlikely to work… I was considering constructing files, e.g., 2.cabal (from 1.cabal and 2.patch) while generating the _repo and then removing the files from _source, so they don't bother the user. However, this approach seems hacky and might not even work since Shake need, AFAIU, is resolved very early, before our CmdBuild routine is run… WDYT?

@michaelpj
Copy link
Collaborator

I'm a bit confused: in that example, have you modified CHaP? At least in current CHaP that file exists, so it's weird that shake would say that it doesn't.

@yvan-sraka
Copy link
Contributor Author

I'm a bit confused: in that example, have you modified CHaP? At least in current CHaP that file exists, so it's weird that shake would say that it doesn't.

My answer was confusing: I didn't test it against CHaP (that doesn't use yet the new revision format) but rather on the README Quickstart example + 2 revisions (1.cabal and 2.patch)!

@andreabedini
Copy link
Member

@yvan-sraka how are you going with this?

@yvan-sraka yvan-sraka force-pushed the patches-revisions branch 4 times, most recently from 77ab848 to a0187da Compare May 23, 2023 08:16
@yvan-sraka
Copy link
Contributor Author

@yvan-sraka how are you going with this?

I just returned from my time-off yesterday, and I was able to merge the changes with the main branch. I will now work on addressing the remaining FIXME/TODO comments in the code. Additionally, I'll be determining an appropriate method to test this feature. :)

@andreabedini
Copy link
Member

Marking this as draft

@andreabedini andreabedini marked this pull request as draft August 11, 2023 05:51
@andreabedini
Copy link
Member

@yvan-sraka are you still interested in finishing this?

Copy link
Contributor Author

@yvan-sraka yvan-sraka left a comment

Choose a reason for hiding this comment

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

I rebased it on upstream and make the code type check, but the logic feels wrong …

@@ -111,20 +110,20 @@ buildAction
( \PreparedPackageVersion {pkgId, pkgTimestamp, cabalFilePath, originalCabalFilePath, cabalFileRevisions} -> do
-- original cabal file, with its timestamp (if specified)
copyFileChanged originalCabalFilePath (outputDir </> "package" </> prettyShow pkgId </> "revision" </> "0" <.> "cabal")
cf <- prepareIndexPkgCabal pkgId (fromMaybe currentTime pkgTimestamp) originalCabalFilePath
cf <- prepareIndexPkgCabal pkgId (Timestamped (fromMaybe currentTime pkgTimestamp) originalCabalFilePath) [] -- FIXME !!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't recall correctly how I should use prepareIndexPkgCabal

Copy link
Member

Choose a reason for hiding this comment

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

IIRC we discussed this privately.

@yvan-sraka yvan-sraka closed this Jun 13, 2024
@yvan-sraka yvan-sraka deleted the patches-revisions branch June 13, 2024 18:24
@amesgen
Copy link
Member

amesgen commented Jun 13, 2024

Is there some fundamental problem with this approach, or do you just don't have time to work on it ATM? @yvan-sraka I would like to try to pick this up in case of the latter.

@yvan-sraka
Copy link
Contributor Author

Is there some fundamental problem with this approach, or do you just don't have time to work on it ATM? @yvan-sraka I would like to try to pick this up in case of the latter.

Yes! It's unlikely I'll have time to work on it before leaving IOG at the end of the month, feel free to give it a try :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use patches for revisions
4 participants