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

[3.7] backport #7445 (Allow package in any location in rule) #7461

Closed
wants to merge 1 commit into from

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Mar 31, 2023

While fixing the package associations in ocaml/opam#5496 I realized that the package stanza is only accepted when it is at the end of the rule stanza.

  • Add test that packages needs to be last, or it will be rejected

    File "dune", line 12, characters 3-10:
    12 | (package a)
    ^^^^^^^
    Error: Unknown action or rule field.
    [1]

While fixing the package associations in ocaml/opam#5496 I realized that the package stanza is only accepted when it is at the end of the rule stanza.

* Add test that `packages` needs to be last, or it will be rejected

    File "dune", line 12, characters 3-10:
    12 |   (package a)
            ^^^^^^^
    Error: Unknown action or rule field.
    [1]

Signed-off-by: Marek Kubica <[email protected]>
Co-authored-by: Etienne Millon <[email protected]>
@emillon emillon added this to the 3.7.1 milestone Mar 31, 2023
@Leonidas-from-XIV
Copy link
Collaborator

I was about to propose this, but I am not entirely sure this makes sense to backport.

To be able to use package everywhere one would need to depend on dune-lang 3.7.1, as dune-lang 3.7 would potentially pick too old of a dune version, so I feel having it only available in 3.8 would make more sense, as it would be available in all 3.8 versions.

@emillon
Copy link
Collaborator Author

emillon commented Mar 31, 2023

Ah, indeed there might be a problem. If your fix makes some dune files valid (that were invalid in older dune versions), it might make sense to version it (and not backport it).

@rgrinberg what do you think? Should the fix in #7445 be versioned or is it ok to keep it unversioned?

(my opinion is that it's unlikely to come up in the wild and that code path may be hard to version, so I'd keep it unversioned)

@rgrinberg
Copy link
Member

rgrinberg commented Mar 31, 2023

I think we should change #7445 to be versioned because the issue will come up with other fields that we'll need to add to that list. Might as well bite the bullet and do it now.

@emillon
Copy link
Collaborator Author

emillon commented Apr 3, 2023

OK, let's version it then (#7471). This disqualifies it for a backport so I'm closing this one. Thanks

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.

3 participants