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

Drop file and module targets #8966

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

andreabedini
Copy link
Collaborator

@andreabedini andreabedini commented May 23, 2023

It looks like the ability of compile sub-component targets (i.e. single files and single modules) has been sitting half backed for many years. This PR indends to remove what is effectively dead code.

cabal-install has a lot of logic to support many different forms of target selectors (see TargetSelector.hs), only to drop the whole thing on the floor when it comes to call Cabal.

https://github.com/haskell/cabal/pull/8966/files#diff-2adae381a2eb2d8c804d5396694a431e1e3babd816a5d2408a62213cf774a893L3132-L3134

Similarly Cabal implements complex matching logic to parse sub-component targets (so it seems to be aware of the functionality it does not yet have) only to give up at the end

https://github.com/haskell/cabal/pull/8966/files#diff-92dccb98376c4c010aaedf4bbea0733e6ab8016318f0038db9ccf4e366fd9e95L1059-L1067

I suggest all this can go, shaving ~1200 loc. I am also confident all this can be done in a simpler way, if and when we decide to have this functionality.


Please include the following checklist in your PR:

Bonus points for added automated tests!

@andreabedini andreabedini self-assigned this May 23, 2023
@andreabedini
Copy link
Collaborator Author

I am afraid this got hit badly by the reformatting :-/ I'll pick it back up another time.

@Kleidukos
Copy link
Member

@andreabedini would you allow me to fix this?

@andreabedini
Copy link
Collaborator Author

@andreabedini would you allow me to fix this?

Absolutely 😂 I don't feel obliged though :-) Do you have any trick up your sleeve to deal with the situation?

@Kleidukos
Copy link
Member

Yes I plan on recreating your changes based on the latest HEAD instead of resolving conflicts by hand. :)

@sheaf
Copy link
Collaborator

sheaf commented Nov 29, 2023

Any updates on this front?

@andreabedini
Copy link
Collaborator Author

Any updates on this front?

This got bitrotten by the codebase formatting. I guess I could give this another go.

@andreabedini andreabedini force-pushed the remove-file-and-module-targets branch 2 times, most recently from c694b92 to 1a6eb9b Compare December 1, 2023 15:27
@andreabedini
Copy link
Collaborator Author

Redoing this wasn't hard in the end, but I must have broken something for the cabal script.

@andreabedini
Copy link
Collaborator Author

Alright I understood what is going on.

In affected test, we have a package with executable foo and main-is: Main.hs. We then check that cabal run Main.hs fails because we say it is meant to run individual haskell files and not files which are part of a package component. The message that the test expects is:

# cabal v2-run
Resolving dependencies...
Error: [Cabal-7070]
The run command can only run an executable as a whole, not files or modules within them, but the target 'Main.hs' refers to the file Main.hs in the executable foo.

After removing sub-component targets, there is no target for "a file part of a component", so that check disappears and we get a more down to earth message:

# cabal v2-run
Error: [Cabal-7121]
Failed extracting script block: `{- cabal:` start marker not found

which is also expected and correct since Main.hs does not have the start marker.

I am inclined to think we can accept this new behaviour: cabal run <file> does less guessing and checking and simply looks at the start marker; if there is a marker cabal uses it, otherwise it complains.

@andreabedini
Copy link
Collaborator Author

@Kleidukos @Mikolaj @ulysses4ever @sheaf what do you think?

@Mikolaj
Copy link
Member

Mikolaj commented Dec 6, 2023

I'm really too ignorant here to comment but, yes, let's drop and delete it all!

@andreabedini andreabedini marked this pull request as ready for review December 7, 2023 02:28
@andreabedini andreabedini changed the title WIP drop file and module targets from Cabal Drop file and module targets from Cabal Dec 7, 2023
@andreabedini andreabedini changed the title Drop file and module targets from Cabal Drop file and module targets Dec 7, 2023
@andreabedini
Copy link
Collaborator Author

I maked this as ready and updated the description.

@ulysses4ever
Copy link
Collaborator

Sorry, busy end of year, haven't gotten any cycles to look into this. On the surface of it sounds fine.

@andreabedini andreabedini force-pushed the remove-file-and-module-targets branch 4 times, most recently from 11f227a to 6916351 Compare December 12, 2023 02:13
Copy link
Collaborator

@sheaf sheaf left a comment

Choose a reason for hiding this comment

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

I agree with your assessment that it's better to remove this effectively dead code rather than hope it will eventually be resuscitated into a working feature.

cabal-install/src/Distribution/Client/CmdTest.hs Outdated Show resolved Hide resolved
Copy link
Contributor

mergify bot commented Jan 25, 2024

refresh

✅ Pull request refreshed

@ulysses4ever
Copy link
Collaborator

Perhaps, it needs a manual rebase on master.

@Mikolaj
Copy link
Member

Mikolaj commented Jan 25, 2024

Isn't it all fine now? Mergify just waiting the 2 days after 2nd review?

@ulysses4ever
Copy link
Collaborator

The reason I mention manual rebase is that GitHub UI says there are conflicts that cannot be automatically resolved. I don't know what Mergify will do about it. We can wait and see.

@Mikolaj
Copy link
Member

Mikolaj commented Jan 25, 2024

@ulysses4ever: I see "This branch has no conflicts with the base branch". Am I looking in the wrong place or do you need to reload the page?

@ulysses4ever
Copy link
Collaborator

@Mikolaj
2024-01-25_10-01-1706194859

@Kleidukos
Copy link
Member

@ulysses4ever welcome to the Twilight Zone I guess
image

@Mikolaj
Copy link
Member

Mikolaj commented Jan 25, 2024

"Impossible" happened, I suppose? Run for the woods?

@geekosaur
Copy link
Collaborator

Actually, if you select the dropdown next to "Update branch", you will find that "Rebase" is grayed out with a message "This branch cannot be rebased due to conflicts". Apparently merge is okay, though.

@Kleidukos
Copy link
Member

Kleidukos commented Jan 25, 2024

oh yeah if you make a merge commit then it will "work"(??)

@geekosaur
Copy link
Collaborator

geekosaur commented Jan 25, 2024

IIRC both would run into conflicts but only github's merge code knows how to deal with them at present? From a local checkout you can fix conflicts and git rebase --continue.

@andreabedini
Copy link
Collaborator Author

image

Here it says no conflicts 🤷‍♂️

@geekosaur
Copy link
Collaborator

Screenshot_2024-01-25_17-45-25

@Mikolaj
Copy link
Member

Mikolaj commented Jan 25, 2024

Actually, if you select the dropdown next to "Update branch", you will find that "Rebase" is grayed out with a message "This branch cannot be rebased due to conflicts". Apparently merge is okay, though.

Oh, so that's it! I think rebase is more strict than merge, so no wonder. If the commits are going to be squashed, no harm in merging (I forgot, does mergify merge or rebase when it's going to squash?), except that rarely some subtle merge errors may occur (e.g., wrong line order) when rebase would just refuse to cooperate.

@ulysses4ever
Copy link
Collaborator

Just when you thought you understood git, git strikes you in the back ...

The only explanation I can think of has to do with multiple commits. Rebase tries to apply every commit in order and can fail on some of them. Whereas merge tries to reconcile the final states of two branches.

II wonder if this is the case that with a single-commit PR rebase and merge will always have the same outcome in terms of conflicts…

And then the whole other trap is GitHub's dreadful UI feature that remembers your last PR action (rebase vs merge vs squash). Thanks to it, everyone can see something different... (Well, no more than three options.)

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jan 28, 2024
@andreabedini
Copy link
Collaborator Author

For security reasons, Mergify can't update this pull request. Try updating locally. GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/validate.yml without workflows permission

Maybe this is something we can fix?

This change removes support for building sub-component targets in
cabal-install, since no versions of Cabal support this feature.

The test RunMainBad is also dropped because without the concept of a
file target, RunMainBad is the same test as ScriptBad.
@andreabedini andreabedini force-pushed the remove-file-and-module-targets branch from 30e2e00 to eda0b22 Compare January 29, 2024 06:04
@mergify mergify bot merged commit 3f4c81f into haskell:master Jan 29, 2024
50 checks passed
@Mikolaj
Copy link
Member

Mikolaj commented Jan 29, 2024

For security reasons, Mergify can't update this pull request. Try updating locally. GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/validate.yml without workflows permission

Maybe this is something we can fix?

Did it fix itself?

@andreabedini
Copy link
Collaborator Author

Did it fix itself?

I rebased manually ☺️

fendor added a commit to fendor/cabal that referenced this pull request Feb 2, 2024
Mikolaj pushed a commit to fendor/cabal that referenced this pull request Feb 8, 2024
mergify bot added a commit that referenced this pull request Feb 8, 2024
…rgets

Revert "Drop sub-component targets (#8966)"
erikd pushed a commit to erikd/cabal that referenced this pull request Apr 22, 2024
This change removes support for building sub-component targets in
cabal-install, since no versions of Cabal support this feature.

The test RunMainBad is also dropped because without the concept of a
file target, RunMainBad is the same test as ScriptBad.
erikd pushed a commit to erikd/cabal that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-review merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants