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

Stricter hygiene by default? #1882

Closed
ejgallego opened this issue Feb 26, 2019 · 13 comments
Closed

Stricter hygiene by default? #1882

ejgallego opened this issue Feb 26, 2019 · 13 comments

Comments

@ejgallego
Copy link
Collaborator

ejgallego commented Feb 26, 2019

The nº 1 problem developers migrating to Dune face in Coq is one of having a dirty build tree:

> [Warning] File vernac.a is both generated by a rule and present in the source tree.
> As a result, the rule is currently ignored, however this will become an error in the future.
> Delete file vernac/vernac.a to get rid of this warning.

IMHO this warning is very confusing and is making new dune users lose a lot of time at first.

Should the warning be finally be made an error, and the message changed to something such as "your source tree is dirty, this is an error. Please remove any object files from it and try again".

[^^ the current wording is super bad I know, but you get the idea]

@ghost
Copy link

ghost commented Feb 26, 2019

We should wait for a major release to change this warning into an error by default, though in the meantime we can add an option in the dune-project file to make it an error right now.

Alternatively, we could also make it an error when using (lang dune x) with x >= 1.8. Technically speaking it is a breaking change for the dune language, but it doesn't feel as bad as changing the behaviour for existing projects.

Thoughts?

@ejgallego
Copy link
Collaborator Author

Any of the two choices would be great for us; maybe the main question is: even today, does this use case makes sense?

Is there anyone relying on picking targets from the source tree for their project to build OK?

@ghost
Copy link

ghost commented Feb 26, 2019

Originally the rule was silently ignored. i.e. (mode fallback) was assumed for all rules. There might be old projects that never added the (mode fallback) field and it doesn't feel right to break their build without notice.

@ejgallego
Copy link
Collaborator Author

I see, other kind of files may indeed be a problem [I was somehow only worried about ML object files]

I guess the warning has been there long enough but without testing it is hard to tell.

Thus in principle an option could be better, on the other hand that would introduce "option creep", I dunno.

@ghost
Copy link

ghost commented Feb 26, 2019

I suggest that we make it an error from version 1.8 of the dune language then: we get the better behavior in newer projects and we don't introduce a new option. It is technically a breaking change in the language which means that upgrading from (lang dune x<1.8) to (lang dune y>=1.8) will sometimes break the build, however the warning as existed for long enough to make this acceptable. Additionally, it doesn't break the build of existing projects, which is the most important property.

@ejgallego
Copy link
Collaborator Author

Yup, it seems not pain-free but on the other hand would any project be affected it pretty easy to add the missing (mode fallback) entry.

@ghost
Copy link

ghost commented Feb 26, 2019

Indeed, do you want to take a stab at implementing this? Something like Path.parent |> Path.drop_optional_build_context |> File_tree.find_dir t.file_tree |> File_tree.Dir.project |> Dune_project.dune_version should give you the version of the dune language in use.

@ejgallego
Copy link
Collaborator Author

I'll be happy to take care of this when my "working on Dune" slot comes, in practice that means in two weeks due to other deadlines so if someone feels like doing it before please do so!

@rgrinberg
Copy link
Member

@ejgallego fixing this doesn't seem like a priority, so we're happy to let you tackle it. Will assign you.

@ejgallego ejgallego self-assigned this Feb 26, 2019
@ghost
Copy link

ghost commented Apr 17, 2019

I ended up doing that in #2068

@ejgallego
Copy link
Collaborator Author

ejgallego commented Apr 17, 2019

Thanks for the update @diml, this was not far from the top of my todo list, sorry I didn't get to do it sooner.

@ghost
Copy link

ghost commented Apr 17, 2019

No problem, it was quite natural to do as part of the other changes

@ejgallego
Copy link
Collaborator Author

Fixed by #2068 then.

@ejgallego ejgallego removed their assignment May 15, 2019
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

No branches or pull requests

2 participants