Skip to content

Do not write 'version' and 'name' fields in opam file #200

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Apr 20, 2020
Merged

Do not write 'version' and 'name' fields in opam file #200

merged 5 commits into from
Apr 20, 2020

Conversation

gpetiot
Copy link
Contributor

@gpetiot gpetiot commented Nov 28, 2019

Should fix #189 if I understood correctly.

@gpetiot gpetiot requested a review from NathanReb November 28, 2019 19:29
@NathanReb
Copy link
Contributor

I gave this a quick look and I think it's a different issue. From what I understood the version: dev field was present in the opam files of irmin's repo. When building the tarball, we sanitize the opam file by removing any existing version field and adding a version: <release_version> field to it.

It appears that this isn't done in dune-release opam pkg which produce the opam file we submit in the opam-repository PR. I'm not sure what the best practice is nowadays and whether the opam file on opam repo should contain the version or not. We need to figure that out but I think a good fix here would be to use the opam file from the tarball as a base for dune-release opam pkg rather than the one in the repo. We could eventually strip the version field there if we don't want it to appear in opam-repo.

I also don't think we should hardcode such things in anycase, if someone which to release a dev version we shouldn't prevent them from doing so, especially given any other string wille be considered a valid version.

@NathanReb
Copy link
Contributor

Just checked with the opam team and best practice is not to have the version field in opam-repository so we should strip it there as well!

@gpetiot gpetiot changed the title Do not write version 'dev' in opam file [WIP] Do not write version 'dev' in opam file Nov 29, 2019
@NathanReb
Copy link
Contributor

We should also strip the name field by the way!

@gpetiot gpetiot changed the title [WIP] Do not write version 'dev' in opam file Do not write 'version' and 'name' fields in opam file Mar 3, 2020
@gpetiot
Copy link
Contributor Author

gpetiot commented Apr 9, 2020

I moved the opam-file-upgrading logics into Pkg and added simple unit tests.

NathanReb
NathanReb reviewed Apr 16, 2020
View reviewed changes
Copy link
Contributor

@NathanReb NathanReb left a comment

Choose a reason for hiding this comment

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

From a quick look it looks good to me. Do you confirm that the opam file that makes it to the tarball is not affected by this change but only the one produced by dune-release opam pkg?

Also a tiny thing that I think we should change here is not to put the upgrade_opam_file function in Pkg. I think this module should be better scoped, it does a bit too much atm. Would you mind moving it to an Opam_file module or something?

I think in the future it might make sense to extract a module to abstract opam files and provide a good testable API on top as I think for now this logic is a bit spread. That's not urgent at all but I think it makes this a good occasion to create such a module.

What do you think?

@gpetiot
Copy link
Contributor Author

gpetiot commented Apr 17, 2020

From a quick look it looks good to me. Do you confirm that the opam file that makes it to the tarball is not affected by this change but only the one produced by dune-release opam pkg?

Indeed it is not affected, I will try to fix that

@gpetiot
Copy link
Contributor Author

gpetiot commented Apr 17, 2020

My last commit also strips these fields from the opam file in the distrib, there is not much code shared with the other opam file right now. (it's not clear to me how to factorize this without introducing some bugs as the code is already different to begin with)

@gpetiot gpetiot requested a review from NathanReb April 17, 2020 16:27
@NathanReb
Copy link
Contributor

Sorry I should have been clearer, I think it's good they still were in the distrib opam files! Let's leave them unchanged for now!

@gpetiot
Copy link
Contributor Author

gpetiot commented Apr 20, 2020

Sorry I should have been clearer, I think it's good they still were in the distrib opam files! Let's leave them unchanged for now!

I wasn't sure I should have asked. Anyway I reverted this commit.

@gpetiot gpetiot merged commit 6814f8b into tarides:master Apr 20, 2020
@gpetiot gpetiot deleted the no-dev-version-in-opam-file branch April 20, 2020 09:16
pitag-ha added a commit to pitag-ha/opam-repository that referenced this pull request Jul 13, 2020
@pitag-ha
[new release] dune-release (1.4.0)
423fdab 
CHANGES:

### Added

- Add a `dune-release config` subcommand to display and edit the global
  configuration (tarides/dune-release#220, @NathanReb).
- Add command `delegate-info` to print information needed by external
  release scripts (tarides/dune-release#221, @pitag-ha)
- Use Curly instead of Cmd to interact with github (tarides/dune-release#202, @gpetiot)
- Add `x-commit-hash` field to the opam file when releasing (tarides/dune-release#224, @gpetiot)
- Add support for common alternative names for the license and
  ChangeLog file (tarides/dune-release#204, @paurkedal)

### Changed

- Command `tag`: improve error and log messages by comparing the provided
  commit with the commit correspondent to the provided tag (tarides/dune-release#226, @pitag-ha)
- Error logs: when an external command fails, include its error message in
  the error message posted by `dune-release` (tarides/dune-release#231, @pitag-ha)
- Error log formatting: avoid unnecessary line-breaks; indent only slightly
  in multi-lines (tarides/dune-release#234, @pitag-ha)
- Linting step of `dune-release distrib` does not fail when opam's `doc` field
  is missing. Do not try to generate nor publish the documentation when opam's
  `doc` field is missing. (tarides/dune-release#235, @gpetiot)

### Deprecated

- Deprecate opam 1.x (tarides/dune-release#195, @gpetiot)

### Fixed

- Separate packages names by spaces in `publish` logs (tarides/dune-release#171, @hannesm)
- Fix uncaught exceptions in distrib subcommand and replace them with proper
  error messages (tarides/dune-release#176, @gpetiot)
- Use the 'user' field in the configuration before inferring it from repo URI
  and handles HTTPS URIs (tarides/dune-release#183, @gpetiot)
- Ignore backup files when looking for README, CHANGES and LICENSE files
  (tarides/dune-release#194, @gpetiot)
- Do not echo input characters when reading token (tarides/dune-release#199, @gpetiot)
- Improve the output of VCS command errors (tarides/dune-release#193, @gpetiot)
- Better error handling when checking opam version (tarides/dune-release#195, @gpetiot)
- Do not write 'version' and 'name' fields in opam file (tarides/dune-release#200, @gpetiot)
- Use Yojson to parse github json response and avoid parsing bugs.
  (tarides/dune-release#177, @gpetiot)
- The `git` command used in `publish doc` should check `DUNE_RELEASE_GIT` (even
  if deprecated) before `PATH`. (tarides/dune-release#242, @gpetiot)
- Adapt the docs to the removal of the `log` subcommand (tarides/dune-release#196, @gpetiot)

### Removed

### Security
@pitag-ha pitag-ha mentioned this pull request Jul 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@NathanReb NathanReb NathanReb approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

dune-release should remove the version field in opam files
2 participants
@gpetiot @NathanReb