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

Fix build from source when a dune-project file is presented in the parent directory #4545

Merged
merged 3 commits into from
Apr 23, 2021

Conversation

kit-ty-kate
Copy link
Member

Fixes #4537

@kit-ty-kate kit-ty-kate force-pushed the fix-dune-root branch 2 times, most recently from 067d733 to cb158dd Compare February 12, 2021 11:07
Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

Thanks!

@rjbou rjbou added this to PR in Progress in Opam 2.1.x via automation Feb 15, 2021
@rjbou rjbou added this to the 2.1.0~beta5 milestone Feb 15, 2021
@rjbou
Copy link
Collaborator

rjbou commented Feb 15, 2021

Just a question, is there a reason to not use DUNE_ARGS to add it one place instead on each dune call ?

@dra27
Copy link
Member

dra27 commented Feb 15, 2021

Just a question, is there a reason to not use DUNE_ARGS to add it one place instead on each dune call ?

🌴 You don't want the user to be able to change it - DUNE_ARGS is for extra arguments, not mandatory ones. It could go in another variable like DUNE_CMD = $(DUNE) --profile=$(...) --root=. but we already didn't do that for --profile

@rjbou rjbou moved this from PR in Progress to PR Finalised in Opam 2.1.x Feb 15, 2021
@rjbou rjbou moved this from PR Finalised to PR in Progress in Opam 2.1.x Feb 15, 2021
Comment on lines +5 to +6
# TODO: Replace --profile=$(DUNE_PROFILE) by --release when we require on dune >= 2.5

Copy link
Member

Choose a reason for hiding this comment

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

This would need to be slightly more complicated - the idea was that you can say make DUNE_PROFILE=dev

Copy link
Member Author

Choose a reason for hiding this comment

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

we could still have something like that:

ifeq ($(DUNE_PROFILE),release)
DUNE_PROFILE_ARG=--release
else
DUNE_PROFILE_ARG=--profile=$(DUNE_PROFILE)
endif

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, that was my slightly more complicated 🙂

doc/Makefile Outdated Show resolved Hide resolved
@dra27
Copy link
Member

dra27 commented Mar 11, 2021

Good to go when CI’s ready

@rjbou
Copy link
Collaborator

rjbou commented Mar 15, 2021

@AltGr can you take a look (and update PR) to be sure to not broke the new dune tests makefiles 😇

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

Rebased and I (throughly) checked all the dune calls - good to go, as long as CI agrees.

Opam 2.1.x automation moved this from PR in Progress to PR Finalised Apr 21, 2021
@dra27 dra27 merged commit 514097b into ocaml:master Apr 23, 2021
Opam 2.1.x automation moved this from PR Finalised to Done Apr 23, 2021
@dra27 dra27 mentioned this pull request May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Opam 2.1.x
  
Done
Development

Successfully merging this pull request may close these issues.

Cannot build opam from sources, when 'dune-project' file is presented in the parent directory
3 participants