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

FAKE keeps reverting the Paket.Restore.targets file updated by Paket. #2332

Closed
teo-tsirpanis opened this issue Jun 10, 2019 · 17 comments
Closed

Comments

@teo-tsirpanis
Copy link
Contributor

Description

With Paket PR 3585 being merged, I updated my Paket.Restore.targets file. However, after I ran FAKE, i saw the file being reverted to its previous edition.

Repro steps

  1. Create a folder with FAKE and Paket.

  2. paket restore.

  3. See the Paket.Restore.targets.

  4. fake build.

  5. See it again.

Expected behavior

Paket.Restore.targets remains the same, and equal to its latest version.

Actual behavior

It gets overwritten by its pre-#3585 edition.

Known workarounds

The developer can exercise care by not commiting the changed file into source control.

However, when using FAKE, the build is performed with the old file, no matter what exists in source control. This makes Paket issues like #3584 or later, still occur.

Related information

  • Version of FAKE (4.X, 5.X): 5.13.7

Here is my guess for the cause of the bug: each Paket version has a specific version of Paket.Restore.targets, and each FAKE version has a specific version of Paket. The former relation is reasonable, given that Paket.Restore.targets and Paket are essentially the same product. However, FAKE does not need to be tied to a version of Paket, so that these two projects can evolve independently.

Fake's Paket is of an earlier version

@matthid
Copy link
Member

matthid commented Jun 10, 2019

See the discussion in #2181
Note that fake gives you a hint when there is a mismatch. So I guess you were ignoring that hint?

@teo-tsirpanis
Copy link
Contributor Author

Oh, I had noticed it in the past. So that was what was it for?

Still I believe this countermeasure is inadequate. Both FAKE and Paket get new versions very fast. The very frequent Paket.Restore.targets changes pollute the source control history enough to not need a similar treatment with paket.dependencies, every time a new version FAKE emerges.

@matthid
Copy link
Member

matthid commented Jun 10, 2019

@teo-tsirpanis It is a feature that fake build does include a paket restore if you use an external paket.dependencies file (see fake docs).

It is quite obvious why newer paket versions can break this particular feature.

However, I use that feature a lot, and I'm not investing any time into making that optional. Additionally, I think excluding that particular feature is technically challenging, considering the Paket.Core architecture.

I'd accept a PR which excludes that particular feature, for example via --no-paket-restore. In this instance, FAKE would not trigger the hint. I can leave that issue open a bit and see if anyone shows interest, but my guess is I'll close it eventually.

Does that clarify the situation?

@BlythMeister
Copy link
Contributor

Can we not "just" upgrade paket.core?
Like running a paket update here?
Happy to raise PR for this in the morning 👍

@teo-tsirpanis
Copy link
Contributor Author

Can we not "just" upgrade paket.core?
Like running a paket update here?
Happy to raise PR for this in the morning 👍

The problem is it just has to be done every time a new Paket version gets released.

@matthid, if I define my dependencies inside the build script, would that solve the issue?

@BlythMeister
Copy link
Contributor

Well that's the issue with any dependency

@BlythMeister
Copy link
Contributor

@matthid. Where in the docs does it say about the restore?
I guess I run a paket restore manually before calling fake, so I would be interested in making the double restore optional lol

@matthid
Copy link
Member

matthid commented Jun 10, 2019

Can we not "just" upgrade paket.core?

The problem is it just has to be done every time a new Paket version gets released.

Indeed, I already do that. It's just that my schedule is to release one time every week at max. I try to do more stable builds. In fact, on some occasions, I couldn't even upgrade and had to report back a bug to Paket. But the more significant issue is that I'm not available 24/7 (therefore the focus on stability). I can give other people release-access to make that process faster. But it remains an issue nontheless.

@matthid, if I define my dependencies inside the build script, would that solve the issue?

I think it will if you use #r "paket:"-inline dependencies, please test. Note that this way you will not update all your dependencies together and you need to delete <build>.fsx.lock to update the dependencies.

I guess I run a paket restore manually before calling fake, so I would be interested in making the double restore optional lol

Note that Paket code already has a fast-path on the second run, so it should be fine. But you can probably simplify your build process.

@matthid. Where in the docs does it say about the restore?

Indeed the docs around this could be improved by including this fact. I probably assumed this to be "obvious" considering you can use fake without Paket ;)

@matthid
Copy link
Member

matthid commented Jun 10, 2019

Honestly imho this is not even an issue just tweak your workflow a bit:
-> Update Fake first and use the paket version it suggests

This way you most likely get even better tested paket versions in the first place.

Can we not "just" upgrade paket.core?

By the way 5.14.0-alpha.1085 is already released including Paket 5.209.1 (which includes the fix you are looking for). The only reason it is alpha (not stable) is that I'm waiting for fsprojects/Paket#3593
So sometimes Paket is the blocker ;)

@BlythMeister
Copy link
Contributor

Maybe fake could automatically set the paket version?
Before restore, ensure paket is locked to the right version in its lock file.
Removes the need to a warning, removes the issues...

@matthid
Copy link
Member

matthid commented Jun 10, 2019

@BlythMeister I don't understand. The issue is that @teo-tsirpanis wants to use the newer version...

@BlythMeister
Copy link
Contributor

Oh right, I didn't follow right. I thought the issue was just the flip flopping targets file...maybe auto pinning could be another feature request 🤣

@matthid
Copy link
Member

matthid commented Jun 22, 2019

Ironically I now hit this in FAKE bootstrapping itself and it cost me a while to realize. The bad news is that the runner was so old that whatever fix I had added in the meantime it wouldn't have helped at all.
Interestingly the relevant logic is soon implemented in paket (fsprojects/Paket#3598) for another reason. So I think we could now easily realize a --no-paket-targets option. However, I'm still not really sold... I'm more on the side to error out if the versions don't match (instead of giving a hint only)

As always: Ideas welcome. The fundamental problem is: Both tools (fake & paket) use the same cache and do the same operations, so they should agree on what the operations are, otherwise subtle errors pop-up independent on whether we write the targets file or not.

@BlythMeister
Copy link
Contributor

Part of me thinks error, part of me thinks use the exe, part of me thinks don't do paket things as part of fake (unless opted in)

@BlythMeister
Copy link
Contributor

Or allow a call to fake and opt out.

Whenever I call dotnet fake in my builds k always get "restore up to date"
Because I use paket to get me fake as a cli tool.

@matthid
Copy link
Member

matthid commented Jun 28, 2019

In the next version of FAKE, you can set PAKET_SKIP_RESTORE_TARGETS=true before running FAKE. All my above objections still hold but if you feel like it is a good idea you can do it ;)

@matthid
Copy link
Member

matthid commented Jun 30, 2019

Closing this again, let's see if this appears again in the future.

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

No branches or pull requests

3 participants