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 Jordan's Spago flowchart for visual overview of workflow #211

Merged
merged 4 commits into from Jun 18, 2019

Conversation

Projects
None yet
2 participants
@JordanMartinez
Copy link
Contributor

commented May 24, 2019

Description of the change

Adds the flowchart I created in my learning repo's Spago-Explained.md file.

PR checklist:

  • Added the change to the "Unreleased" section of the changelog

Ok @f-f Let's discuss!

@JordanMartinez JordanMartinez force-pushed the JordanMartinez:addSpagoFlowchart branch from d0667d4 to 007278d May 24, 2019

@f-f

This comment has been minimized.

Copy link
Member

commented May 26, 2019

@JordanMartinez thanks!

As I mentioned in Slack, I think we should adjust the diagram a bit, because:

  • The global cache is coming with #188 (waiting on #212 before merging, but it's otherwise ready), and I think we'd need to include it here
  • I have the feeling the diagram is trying to cover two otherwise separate aspects:
    1. what the user does or wants to do (additional concern here: should we cover all the complete tree of possibilities or just the main user flows?)
    2. what happens under the hood
      I don't have a preference on how to solve this, as it's a tradeoff: if we have a clear separation but keep everything in the same diagram then it might be comprehensive but overwhelming, while if we split into two diagrams then we might need a third one to tie the other two together. Makes sense?
@JordanMartinez

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

The global cache is coming with #188 (waiting on #212 before merging, but it's otherwise ready), and I think we'd need to include it here

Agreed.

I have the feeling the diagram is trying to cover two otherwise separate aspects:

I think the 'what happens under the hood' is the basic idea I'm trying to explain here. It helps one understand the larger context and why Spago uses the terms it does.

We could cover the typical user workflow in a separate flowchart or just via a step-by-step process similar to what I did in https://github.com/JordanMartinez/purescript-jordans-reference/blob/latestRelease/02-Build-Tools/05-Spago--Project-Workflow.md
That's overall easier to update than a flowchart and it follows the general flow a new user would take to create and use a Spago-based project

@JordanMartinez

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

@f-f Can you ping me on this PR once all dependent work is finished? Then I can get back to it.

@f-f

This comment has been minimized.

Copy link
Member

commented May 28, 2019

Can you ping me on this PR once all dependent work is finished? Then I can get back to it.

@JordanMartinez sorry for delay, the global cache thing was kind of a big brick but now everything has been merged 🎉
I also added an explanation about the inner workings of the global cache which might be useful for the diagram

I think the 'what happens under the hood' is the basic idea I'm trying to explain here. It helps one understand the larger context and why Spago uses the terms it does.

Wonderful! Then how about splitting this in two sections under Explanations:

  • one called "What happens when you do spago build", that contains a diagram of all the steps (we can use spago build if we want to cover install+build, or spago bundle-module if we want to cover install+build+bundle)
  • the other called "Spago's Glossary" that explains all the terms that might be unfamiliar to the reader

We could cover the typical user workflow in a separate flowchart or just via a step-by-step process similar to what I did in https://github.com/JordanMartinez/purescript-jordans-reference/blob/latestRelease/02-Build-Tools/05-Spago--Project-Workflow.md
That's overall easier to update than a flowchart and it follows the general flow a new user would take to create and use a Spago-based project

Nice, this is basically what we already have in the "Super quick tutorial", except you also have a section about Parcel, which we could add here!

@JordanMartinez

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

@f-f I'm going to check out the latest Spago release and then get back to this. However, it's going to be a few more days before I do anything.

@JordanMartinez

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2019

I thought I'd have more time today (but that doesn't seem to be the case), so looks like this will need to be pushed back again.

I have looked at how I could do the flowchart and below was my first draft at just understanding the global cache's workflow. It's a "static" image that shows where code comes from:
static-dependencies-source

@f-f

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

@JordanMartinez that's basically it! Some small notes:

  • the code in the global cache is not git cloned, but we just fetch the tarball. This saves a lot of space and time
  • the dependencies that are found in the global cached are not "pointed to", but the files get copied (I guess we could just point to them, but this is how it works right now)
  • the arrows are a bit confusing, as they mean "download" in the left part and "compile" in the right part
@JordanMartinez

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

the code in the global cache is not git cloned, but we just fetch the tarball. This saves a lot of space and time

Does this mean code can only be fetched from GitHub (as opposed to things like BitBucket/Gitlab)?

the dependencies that are found in the global cached are not "pointed to", but the files get copied (I guess we could just point to them, but this is how it works right now)

I think copying them makes more sense. If I deleted the global cache file, shouldn't my code still compile? If we just pointed there, that would no longer be the case. Although this is also true with local dependencies, it's fine in that situation because the end-user is the one adding them and is thus responsible for maintaining them.

the arrows are a bit confusing, as they mean "download" in the left part and "compile" in the right part

Yeah. This isn't a first draft to what I want to produce, but more just a way for me to ensure I'm understanding it correctly myself.

@f-f

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Does this mean code can only be fetched from GitHub (as opposed to things like BitBucket/Gitlab)?

@JordanMartinez yes, we literally hardcode GitHub here:

globallyCache (packageName, url, ref) downloadDir metadata cacheableCallback notCacheableCallback = do
case (Text.stripPrefix "https://github.com/" url)
>>= (Text.stripSuffix ".git")
>>= (Just . Text.split (== '/')) of
Just [owner, repo] -> do
case (isTag <|> isCommit) of
Nothing -> notCacheableCallback -- TODO: nice error?
Just _ -> do
let archiveUrl = "https://github.com/" <> owner <> "/" <> repo <> "/archive/" <> ref <> ".tar.gz"
fetchTarball downloadDir archiveUrl
Just resultDir <- Turtle.fold (Turtle.ls $ Turtle.decodeString downloadDir) Fold.head
cacheableCallback $ Turtle.encodeString resultDir
where
_ -> do
echo $ "Warning: was not able to match url with GitHub ones: " <> url
notCacheableCallback -- TODO: error?

Though I know that GitLab also has a url for tarballs, so if we ever get non-GitHub packages in the package set we can probably extend this.
Right now the non-GitHub packages are just cloned normally, so it's not a problem - this is just an optimization to save space and download time, as git operations will be super slow especially on big repos

@JordanMartinez

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

I updated my original flowchart to below. It doesn't quite follow what we've described above, but it's easier to delete things from here.

spago-flowchart

@f-f

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

@JordanMartinez looks good. Some further comments:

  • The arrows are a bit confusing to me: the whole diagram has a "geographical" feeling to it - i.e. it shows the locations of things neatly classified into boxes - so one would expect that the arrows would mean "geographical transfer". And that's indeed the case for many steps (1, 4, 5), but not for others, where the arrows become transitions in a flowchart (2, 3?)
  • I think it would be useful to distinguish between "operations that the user does" (2a, 2b) and "operations that spago does" (everything else). Since the only outlier here is 2 we could move it from being a "step" in the process to just being a "note for users"? This would probably also solve the point above
  • In 4d there's no "linking" happening, the filepath of the current location is just passed to purs which reads them from their original location. That particular wording might confuse readers and make them think there's actual symlinking going on
  • 4b and 4c should not be independent but chained: 4b should go from the global cache to the local cache, not from the remote. This is because if a cacheable dependency is not globally cached then it will be, and at that point it just becomes a "Remote but already cached dependency" (that is, I think the "Remote but cacheable dependency" should not appear)
@JordanMartinez

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

Sorry for my lack of response. I was at LambdaConf this past week/weekend.

The arrows are a bit confusing to me: the whole diagram has a "geographical" feeling to it - i.e. it shows the locations of things neatly classified into boxes - so one would expect that the arrows would mean "geographical transfer". And that's indeed the case for many steps (1, 4, 5), but not for others, where the arrows become transitions in a flowchart (2, 3?)

I think it would be useful to distinguish between "operations that the user does" (2a, 2b) and "operations that spago does" (everything else). Since the only outlier here is 2 we could move it from being a "step" in the process to just being a "note for users"? This would probably also solve the point above

Perhaps I should visually distinguish one group of arrows from another? In such a case, black arrows could represent copying from one place to another, and blue arrows could represent transitions (or some other color scheme like that).

In 4d there's no "linking" happening, the filepath of the current location is just passed to purs which reads them from their original location. That particular wording might confuse readers and make them think there's actual symlinking going on

Ah... Good to know.

4b and 4c should not be independent but chained: 4b should go from the global cache to the local cache, not from the remote. This is because if a cacheable dependency is not globally cached then it will be, and at that point it just becomes a "Remote but already cached dependency" (that is, I think the "Remote but cacheable dependency" should not appear)

Hmm... Ok. I'll have to rethink how to do that one.

I'll try to finish this PR by tomorrow.

@f-f

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

@JordanMartinez no worries, hope you enjoyed the conf! 🙂

Perhaps I should visually distinguish one group of arrows from another? In such a case, black arrows could represent copying from one place to another, and blue arrows could represent transitions (or some other color scheme like that).

I thought about that, but maybe it's better to just let go of the arrows. After all if you need to modify your package set the flow is straight: you add the packages there and eventually verify. So would it be possible to add an "info box" (maybe joined by a dotted line to the relevant step) with something like

Here you might need to further configure the package set, by:

  • adding packages or
  • override existing ones

If so, then..

Another thing I noticed is that there should be some instructions on how to edit and regenerate the diagram. We already have a section with instructions on how to hack on Spago so they could be added there

@JordanMartinez

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

How's this?
spago-flowchart

@JordanMartinez

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

nother thing I noticed is that there should be some instructions on how to edit and regenerate the diagram. We already have a section with instructions on how to hack on Spago so they could be added there

Good point. I'll work on that, too.

@JordanMartinez JordanMartinez force-pushed the JordanMartinez:addSpagoFlowchart branch from 007278d to 82ed407 Jun 12, 2019

@JordanMartinez

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

A few other questions I have...

Where should I store the .graphml and exported image files? Should that be stored in the root directory or in its own diagrams or flowcharts directory?

@JordanMartinez

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

Sigh... That whitespace issue again... Let me fix it.

@JordanMartinez JordanMartinez force-pushed the JordanMartinez:addSpagoFlowchart branch from 82ed407 to 2547b53 Jun 12, 2019

@f-f

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

@JordanMartinez looks great!

Where should I store the .graphml and exported image files? Should that be stored in the root directory or in its own diagrams or flowcharts directory?

Let's go with diagrams

Sigh... That whitespace issue again... Let me fix it.

I actually gave up on that markdown feature, so don't worry about whitespace refactoring 🙂

@f-f

f-f approved these changes Jun 16, 2019

Show resolved Hide resolved README.md Outdated

JordanMartinez and others added some commits Jun 17, 2019

@f-f

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

@JordanMartinez thank you very much for doing this! 👏

@f-f f-f merged commit 19e3725 into spacchetti:master Jun 18, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@JordanMartinez

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

Np! Thanks for working on this project. It's been very useful!

@JordanMartinez JordanMartinez deleted the JordanMartinez:addSpagoFlowchart branch Jun 19, 2019

elliotdavies pushed a commit to elliotdavies/spago that referenced this pull request Jul 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.