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

Add --offline flag #841

Closed
wants to merge 12 commits into from
Closed

Add --offline flag #841

wants to merge 12 commits into from

Conversation

JordanMartinez
Copy link
Contributor

@JordanMartinez JordanMartinez commented Jan 25, 2022

Description of the change

Fixes #575

Checklist:

  • Added the change to the "Unreleased" section of the changelog
  • Added some example of the new feature to the README
  • Added a test for the contribution (if applicable)

P.S.: the above checks are not compulsory to get a change merged, so you may skip them. However, taking care of them will result in less work for the maintainers and will be much appreciated 😊

@JordanMartinez
Copy link
Contributor Author

I'm not really sure what should be added to the readme (if anything). It's more likely that Nix tools would update their readme to show how to use this command in their workflow.

@f-f
Copy link
Member

f-f commented Jan 27, 2022

I think we could merge this already, but I'd first like that we figure out a good testing story for this, because if we can't properly test that the changes we introduce are going to continue working, then we are deemed to break them.

I don't have many ideas about this though - nix-shell doesn't seem to offer any switch to disable the network 🤔

@JordanMartinez
Copy link
Contributor Author

So.... how should I make progress on this?

@thomashoneyman
Copy link
Member

Could you mock all network calls so they fail the test right away if called? You could use these mocked calls for the offline mode test only.

@f-f
Copy link
Member

f-f commented Feb 5, 2022

Oh, it looks like we can call a process with unshare -r -n to prevent it from reaching the network (on linux at least. We can run the offline tests only on Linux, I'm fine with this)

So I'd say the next step on this is to fixup a test infrastructure that runs the tests for the offline feature in such wrapper

@JordanMartinez
Copy link
Contributor Author

Hmm... I should add a second test that verifies spago build fails when we drop the offline flag but still use unshare -r -n...

@JordanMartinez
Copy link
Contributor Author

Dropping the offline flag but still running unshare -r -n does not produce the expected failure. I wondered as much. AFAICT, spago build should always work so long as the dependencies are stored in the cache folder. I think the real issue is that running Spago in a Nix environment means the .cache folder doesn't contain these dependencies and so triggers the network call.

@f-f
Copy link
Member

f-f commented Feb 6, 2022

@JordanMartinez I see. The location of both Dhall and Spago folders is dependent on the value of the XDG_CACHE_DIR environment variable. We could try to set that do a temp directory when running these tests, so that they would start from a clean slate?

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.

Add --offline flag
3 participants