Skip to content

github-ci: add fpm support #508

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

Merged
merged 6 commits into from
Sep 16, 2021
Merged

github-ci: add fpm support #508

merged 6 commits into from
Sep 16, 2021

Conversation

zoziha
Copy link
Contributor

@zoziha zoziha commented Sep 1, 2021

Description

We have this urgent need for fortran-stdlib. We hope that stdlib and fpm can interact, and stdlib-fpm will be placed in the fortran-lang/stdlib repository (branch).
The CI scheme here comes from @LKedward (https://github.com/LKedward/stdlib-fpm). I have made some modifications to adapt to specific technical requirements. I am not very familiar with CI. I need to admit that the CI scheme this time has shortcomings, but I believe we can iterate and update it.

User efficiency

In order to improve the efficiency of users of stdlib, we can set the stdlib-fpm branch for stdlib, allowing users to add the following reference in their projects:

[dependencies]
stdlib = { git = "https://github.com/fortran-lang/stdlib", branch = "stdlib-fpm" }

@zoziha
Copy link
Contributor Author

zoziha commented Sep 6, 2021

I checked again, the entire CI process seems to be no problem, except that the fpm test (release) task takes a long time.

If you have any questions, you can give me feedback and I will quickly correct it.

In addition, once we think there is no problem with this CI, that we can pass it. We need to create a branch, stdlib-fpm, to support fpm, see fortran-fans : stdlib-fpm for my stdlib-fpm branch.

@awvwgk
Copy link
Member

awvwgk commented Sep 6, 2021

Depending on the commit action it will create the branch all by itself, the one used for deploying fpm's documentation will create the branch automatically for example (https://github.com/marketplace/actions/deploy-to-github-pages).

@awvwgk
Copy link
Member

awvwgk commented Sep 6, 2021

Also, shouldn't the deployment only happen on push actions for the default branch rather than on PRs as well?

For running the action on a PR we don't have to checkout the stdlib-fpm branch at all but just deploy the converted project to a new directory and test it. The checkout is only required for updating the branch on push and this shouldn't be required when running on PRs.

@zoziha
Copy link
Contributor Author

zoziha commented Sep 6, 2021

Depending on the commit action it will create the branch all by itself, the one used for deploying fpm's documentation will create the branch automatically for example (https://github.com/marketplace/actions/deploy-to-github-pages).

I think stdlib-fpm needs the existence of the fpm.toml file. I think there are two ways that I can think of:

  1. Write fpm.toml in stdlib-master. When CI is running, copy the fpm.toml file in stdlib-master to stdlib-fpm. In this case, fpm.toml needs to be maintained in stdlib-master, although I think fpm.toml may not need to be updated frequently.
  2. Manually create a stdlib-fpm branch, and manually submit fpm.toml to stdlib-fpm. When CI is running, it is also possible. fpm.toml needs to be maintained in stdlib-fpm in this case.

Thank you for your suggestion @awvwgk , I think both methods are fine.
I prefer the first option. If everyone agrees, I will add a file: fpm.toml to stdlib-master(./ci/fpm.toml).

Also, shouldn't the deployment only happen on push actions for the default branch rather than on PRs as well?

# When a PR occurs, the fpm package of `stdlib` will be generated.
on: [push, pull_request]

Yes, both PR and push will activate fpm-deployment.yml, and I will remove the incorrect comment.

@zoziha zoziha marked this pull request as ready for review September 6, 2021 12:25
@zoziha

This comment has been minimized.

@LKedward
Copy link
Member

Great stuff @zoziha - thanks for implementing this!

The CI is failing currently, because it is trying to deploy the stdlib-fpm package but we don't want to deploy for PRs, only on pushes to master - see my suggestion above.

@zoziha
Copy link
Contributor Author

zoziha commented Sep 13, 2021

@awvwgk @LKedward Thank you very much for your answers, now I understand what you mean.
I resubmitted a git push, hope there is no problem.

@LKedward LKedward requested a review from awvwgk September 13, 2021 10:30
awvwgk
awvwgk previously requested changes Sep 13, 2021
awvwgk and others added 3 commits September 13, 2021 21:11
- fix all shellcheck warning
- don't write into source directory
- allow adjusting of destination directory
Minor adjustments to deployment script
@LKedward
Copy link
Member

Thanks for reviewing and updating Sebastian @awvwgk. It would be good to get another reviewer on this to move it forward.

@LKedward LKedward dismissed awvwgk’s stale review September 15, 2021 10:44

Changes applied by reviewer

Copy link
Member

@LKedward LKedward left a comment

Choose a reason for hiding this comment

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

Latest updates to fpm-deployment.sh look good to me 👍

@zoziha

This comment has been minimized.

Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

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

Great and highly needed addition. It will benefit to both stdlib and fpm IMO. Thank you @zoziha .
I approve this PR, pending minor revisions.

@zoziha
Copy link
Contributor Author

zoziha commented Sep 15, 2021

I modified the corresponding content of README.md. @jvdp1 😉

@urbanjost
Copy link

Seems like stdlib should go onto the fpm registry but I do not see it there (yet?)

@jvdp1
Copy link
Member

jvdp1 commented Sep 16, 2021

With 3 approvals, I will merge this PR. Thank you @zoziha

@jvdp1 jvdp1 merged commit 608d5a2 into fortran-lang:master Sep 16, 2021
@zoziha zoziha deleted the add_fpm_ci2 branch September 17, 2021 05:44
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.

5 participants